linux-kernel - Re: [PATCH v3 2/3] zram: fix deadlock with sysfs attribute usage and driver removal

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]

Message-ID: <YNIPiDb3TPamz5cF@kroah.com>
Date:   Tue, 22 Jun 2021 18:27:52 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Luis Chamberlain <mcgrof@...nel.org>
Cc:     minchan@...nel.org, jeyu@...nel.org, ngupta@...are.org,
        sergey.senozhatsky.work@...il.com, axboe@...nel.dk,
        mbenes@...e.com, jpoimboe@...hat.com, tglx@...utronix.de,
        keescook@...omium.org, jikos@...nel.org, rostedt@...dmis.org,
        peterz@...radead.org, linux-block@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/3] zram: fix deadlock with sysfs attribute usage and
 driver removal

On Tue, Jun 22, 2021 at 08:27:13AM -0700, Luis Chamberlain wrote:
> On Tue, Jun 22, 2021 at 09:41:23AM +0200, Greg KH wrote:
> > On Mon, Jun 21, 2021 at 04:36:34PM -0700, Luis Chamberlain wrote:
> > > +	ssize_t __ret; \
> > > +	if (!try_module_get(THIS_MODULE)) \
> > 
> > try_module_get(THIS_MODULE) is always racy and probably does not do what
> > you want it to do.  You always want to get/put module references from
> > code that is NOT the code calling these functions.
> 
> In this case, we want it to trump module removal if it succeeds. That's all.

True, but either you stop the race, or you do not right?  If you are so
invested in your load/unload test, this should show up with this code
eventually as well.

> > > +		return -ENODEV; \
> > > +	__ret = _name ## _store(dev, attr, buf, len); \
> > > +	module_put(THIS_MODULE); \
> > 
> > This too is going to be racy.
> > 
> > While fun to poke at, I still think this is pointless.
> 
> If you have a better idea, which does not "DOS" module removal, please
> let me know!

I have yet to understand why you think that the load/unload in a loop is
a valid use case.

thanks,

greg k-h