Message ID | 20090209151830.GC6018@dhcp35.suse.cz |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
On Tuesday 10 February 2009 01:48:31 Michal Hocko wrote: > Based on this change, would it make sense to update sys_accept to change > __module_get to try_module_get like in the following patch? I don't think so: > /* > - * We don't need try_module_get here, as the listening socket (sock) > - * has the protocol module (sock->ops->owner) held. > + * Socket's owner cannot be in unloading path because there > + * must be at least one listening reference > */ > - __module_get(newsock->ops->owner); > + if (unlikely(!try_module_get(newsock->ops->owner))) > + BUG(); rmmod --wait can make try_module_get fail even if the reference count isn't zero. But in this case, we should return an error from the accept call; presumably the admin really doesn't want us to keep using the module... Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 10, 2009 at 01:45:07PM +1030, Rusty Russell wrote: > On Tuesday 10 February 2009 01:48:31 Michal Hocko wrote: > > Based on this change, would it make sense to update sys_accept to change > > __module_get to try_module_get like in the following patch? > > I don't think so: > > > /* > > - * We don't need try_module_get here, as the listening socket (sock) > > - * has the protocol module (sock->ops->owner) held. > > + * Socket's owner cannot be in unloading path because there > > + * must be at least one listening reference > > */ > > - __module_get(newsock->ops->owner); > > + if (unlikely(!try_module_get(newsock->ops->owner))) > > + BUG(); > > rmmod --wait can make try_module_get fail even if the reference count isn't > zero. But in this case, we should return an error from the accept call; > presumably the admin really doesn't want us to keep using the module... > I was thinking about this for a while too, but I did not find a valid error value for this case, the current accept syscall API only allow few error codes and I think we should not break this. Maybe -ECONNABORTED could be used but it doesn't fit 100% and applications may interpret this in a wrong way. If the admin decide to remove the module, he should also stop the services needing this resource. In this case the effect of __module_get(newsock->ops->owner); is ok, if the service is still in use, the module will not be removed and still does the expected work, if all sockets are closed the module refcount will reach zero and the module will go away.
On Tue 10-02-09 13:45:07, Rusty Russell wrote: > On Tuesday 10 February 2009 01:48:31 Michal Hocko wrote: > > Based on this change, would it make sense to update sys_accept to change > > __module_get to try_module_get like in the following patch? > > I don't think so: > > > /* > > - * We don't need try_module_get here, as the listening socket (sock) > > - * has the protocol module (sock->ops->owner) held. > > + * Socket's owner cannot be in unloading path because there > > + * must be at least one listening reference > > */ > > - __module_get(newsock->ops->owner); > > + if (unlikely(!try_module_get(newsock->ops->owner))) > > + BUG(); > > rmmod --wait can make try_module_get fail even if the reference count isn't > zero. OK, I though that rmmod --wait waits for refcount==0 and then changes the state. > But in this case, we should return an error from the accept call; > presumably the admin really doesn't want us to keep using the module... > > Thanks, > Rusty.
On Tuesday 10 February 2009 21:01:37 Michal Hocko wrote: > On Tue 10-02-09 13:45:07, Rusty Russell wrote: > > On Tuesday 10 February 2009 01:48:31 Michal Hocko wrote: > > > Based on this change, would it make sense to update sys_accept to change > > > __module_get to try_module_get like in the following patch? > > > > I don't think so: > > > > > /* > > > - * We don't need try_module_get here, as the listening socket (sock) > > > - * has the protocol module (sock->ops->owner) held. > > > + * Socket's owner cannot be in unloading path because there > > > + * must be at least one listening reference > > > */ > > > - __module_get(newsock->ops->owner); > > > + if (unlikely(!try_module_get(newsock->ops->owner))) > > > + BUG(); > > > > rmmod --wait can make try_module_get fail even if the reference count isn't > > zero. > > OK, I though that rmmod --wait waits for refcount==0 and then changes > the state. No, it has to stop all future use, otherwise it's useless under load. Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/socket.c b/net/socket.c index 35dd737..d0d4c92 100644 --- a/net/socket.c +++ b/net/socket.c @@ -1444,10 +1444,11 @@ SYSCALL_DEFINE4(accept4, int, fd, struct sockaddr __user *, upeer_sockaddr, newsock->ops = sock->ops; /* - * We don't need try_module_get here, as the listening socket (sock) - * has the protocol module (sock->ops->owner) held. + * Socket's owner cannot be in unloading path because there + * must be at least one listening reference */ - __module_get(newsock->ops->owner); + if (unlikely(!try_module_get(newsock->ops->owner))) + BUG(); newfd = sock_alloc_fd(&newfile, flags & O_CLOEXEC); if (unlikely(newfd < 0)) {