linux-kernel - Re: [PATCH v8 11/12] zram: fix crashes with cpu hotplug multistate

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:   Wed, 20 Oct 2021 08:43:37 +0200 (CEST)
From:   Miroslav Benes <mbenes@...e.cz>
To:     Ming Lei <ming.lei@...hat.com>
cc:     Luis Chamberlain <mcgrof@...nel.org>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>, tj@...nel.org,
        gregkh@...uxfoundation.org, akpm@...ux-foundation.org,
        minchan@...nel.org, jeyu@...nel.org, shuah@...nel.org,
        bvanassche@....org, dan.j.williams@...el.com, joe@...ches.com,
        tglx@...utronix.de, keescook@...omium.org, rostedt@...dmis.org,
        linux-spdx@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-block@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
        live-patching@...r.kernel.org
Subject: Re: [PATCH v8 11/12] zram: fix crashes with cpu hotplug multistate

On Tue, 19 Oct 2021, Ming Lei wrote:

> On Tue, Oct 19, 2021 at 08:23:51AM +0200, Miroslav Benes wrote:
> > > > By you only addressing the deadlock as a requirement on approach a) you are
> > > > forgetting that there *may* already be present drivers which *do* implement
> > > > such patterns in the kernel. I worked on addressing the deadlock because
> > > > I was informed livepatching *did* have that issue as well and so very
> > > > likely a generic solution to the deadlock could be beneficial to other
> > > > random drivers.
> > > 
> > > In-tree zram doesn't have such deadlock, if livepatching has such AA deadlock,
> > > just fixed it, and seems it has been fixed by 3ec24776bfd0.
> > 
> > I would not call it a fix. It is a kind of ugly workaround because the 
> > generic infrastructure lacked (lacks) the proper support in my opinion. 
> > Luis is trying to fix that.
> 
> What is the proper support of the generic infrastructure? I am not
> familiar with livepatching's model(especially with module unload), you mean
> livepatching have to do the following way from sysfs:
> 
> 1) during module exit:
> 	
> 	mutex_lock(lp_lock);
> 	kobject_put(lp_kobj);
> 	mutex_unlock(lp_lock);
> 	
> 2) show()/store() method of attributes of lp_kobj
> 	
> 	mutex_lock(lp_lock)
> 	...
> 	mutex_unlock(lp_lock)

Yes, this was exactly the case. We then reworked it a lot (see 
958ef1e39d24 ("livepatch: Simplify API by removing registration step"), so 
now the call sequence is different. kobject_put() is basically offloaded 
to a workqueue scheduled right from the store() method. Meaning that 
Luis's work would probably not help us currently, but on the other hand 
the issues with AA deadlock were one of the main drivers of the redesign 
(if I remember correctly). There were other reasons too as the changelog 
of the commit describes.

So, from my perspective, if there was a way to easily synchronize between 
a data cleanup from module_exit callback and sysfs/kernfs operations, it 
could spare people many headaches.
 
> IMO, the above usage simply caused AA deadlock. Even in Luis's patch
> 'zram: fix crashes with cpu hotplug multistate', new/same AA deadlock
> (hot_remove_store() vs. disksize_store() or reset_store()) is added
> because hot_remove_store() isn't called from module_exit().
> 
> Luis tries to delay unloading module until all show()/store() are done. But
> that can be obtained by the following way simply during module_exit():
> 
> 	kobject_del(lp_kobj); //all pending store()/show() from lp_kobj are done,
> 						  //no new store()/show() can come after
> 						  //kobject_del() returns	
> 	mutex_lock(lp_lock);
> 	kobject_put(lp_kobj);
> 	mutex_unlock(lp_lock);

kobject_del() already calls kobject_put(). Did you mean __kobject_del(). 
That one is internal though.
 
> Or can you explain your requirement on kobject/module unload in a bit
> details?

Does the above makes sense?

Thanks

Miroslav