netdev - Re: [RFC] Suspicious bug in module refcounting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 10 Feb 2009 04:42:50 +0100
From:	Karsten Keil <kkeil@...e.de>
To:	Rusty Russell <rusty@...tcorp.com.au>
Cc:	Michal Hocko <mhocko@...e.cz>,
	"David S. Miller" <davem@...emloft.net>,
	linux-kernel@...r.kernel.org,
	richard kennedy <richard@....demon.co.uk>,
	Dan Williams <dan.j.williams@...el.com>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Russell King <rmk+kernel@....linux.org.uk>,
	dwmw2@...radead.org, Scott Wood <scottwood@...escale.com>,
	netdev@...r.kernel.org, Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [RFC] Suspicious bug in module refcounting

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.

-- 
Karsten Keil
SuSE Labs
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html