netdev
[Top] [All Lists]

Re: [PATCH 2.5.69] Bug in sys_accept() module ref counts

To: Sridhar Samudrala <sri@xxxxxxxxxx>
Subject: Re: [PATCH 2.5.69] Bug in sys_accept() module ref counts
From: Arnaldo Carvalho de Melo <acme@xxxxxxxxxxxxxxxx>
Date: Tue, 6 May 2003 21:14:19 -0300
Cc: davem@xxxxxxxxxx, netdev@xxxxxxxxxxx
In-reply-to: <Pine.LNX.4.44.0305061223090.1348-100000@dyn9-47-18-140.beaverton.ibm.com>
Organization: Conectiva S.A.
References: <Pine.LNX.4.44.0305061223090.1348-100000@dyn9-47-18-140.beaverton.ibm.com>
Sender: netdev-bounce@xxxxxxxxxxx
User-agent: Mutt/1.5.4i
Em Tue, May 06, 2003 at 12:28:41PM -0700, Sridhar Samudrala escreveu:
> 
> I think there is a bug in the recent changes to sys_accept() to implement 
> module ref counts.

Yes, well spotted, small comment below, I'll be sending this patch
to DaveM, thanks a lot!
 
> module_put() gets called twice on error. Once via the explicit module_put and
> the second via sock_release(). Also i think we should do a __module_get() 
> with 
> newsock's owner(although same as the original listening sock).
> 
> The following patch against 2.5.69 should fix the problem.
> 
> Thanks
> Sridhar
> -------------------------------------------------------------------------------
> diff -Nru a/net/socket.c b/net/socket.c
> --- a/net/socket.c    Tue May  6 12:14:35 2003
> +++ b/net/socket.c    Tue May  6 12:14:35 2003
> @@ -1280,26 +1280,26 @@
>        * We don't need try_module_get here, as the listening socket (sock)
>        * has the protocol module (sock->ops->owner) held.
>        */
> -     __module_get(sock->ops->owner);
> +     __module_get(newsock->ops->owner);

This one is OK, but the two operations are the same, so the effect, as well,
is the same, but for correctness, better have it with newsock.
  
>       err = sock->ops->accept(sock, newsock, sock->file->f_flags);
>       if (err < 0)
> -             goto out_module_put;
> +             goto out_release;
>  
>       if (upeer_sockaddr) {
>               if(newsock->ops->getname(newsock, (struct sockaddr *)address, 
> &len, 2)<0) {
>                       err = -ECONNABORTED;
> -                     goto out_module_put;
> +                     goto out_release;
>               }
>               err = move_addr_to_user(address, len, upeer_sockaddr, 
> upeer_addrlen);
>               if (err < 0)
> -                     goto out_module_put;
> +                     goto out_release;
>       }
>  
>       /* File flags are not inherited via accept() unlike another OSes. */
>  
>       if ((err = sock_map_fd(newsock)) < 0)
> -             goto out_module_put;
> +             goto out_release;
>  
>       security_socket_post_accept(sock, newsock);
>  
> @@ -1307,8 +1307,6 @@
>       sockfd_put(sock);
>  out:
>       return err;
> -out_module_put:
> -     module_put(sock->ops->owner);
>  out_release:
>       sock_release(newsock);
>       goto out_put;
> 

<Prev in Thread] Current Thread [Next in Thread>