Re: [PATCH] Avoid that SCSI device removal through sysfs triggers a deadlock

[ebiederm@xmission.com (Eric W. Biederman)]

James Bottomley <jejb@linux.vnet.ibm.com> writes:

> On Tue, 2016-11-08 at 08:52 -0800, Bart Van Assche wrote:
>> On 11/08/2016 07:28 AM, James Bottomley wrote:
>> > On Mon, 2016-11-07 at 16:32 -0800, Bart Van Assche wrote:
>> > > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
>> > > index cf4c636..44ec536 100644
>> > > --- a/fs/kernfs/dir.c
>> > > +++ b/fs/kernfs/dir.c
>> > > @@ -1410,7 +1410,7 @@ int kernfs_remove_by_name_ns(struct
>> > > kernfs_node
>> > > *parent, const char *name,
>> > >  	mutex_lock(&kernfs_mutex);
>> > > 
>> > >  	kn = kernfs_find_ns(parent, name, ns);
>> > > -	if (kn)
>> > > +	if (kn && !(kn->flags & KERNFS_SUICIDED))
>> > 
>> > Actually, wrong flag, you need KERNFS_SUICIDAL.  The reason is that
>> > kernfs_mutex is actually dropped half way through __kernfs_remove, 
>> > so KERNFS_SUICIDED is not set atomically with this mutex.
>> 
>> Hello James,
>> 
>> Sorry but what you wrote is not correct.
>
> I think you agree it is dropped.  I don't need to add the bit about the
> reacquisition because the race is mediated by the first acquisition not
> the second one, if you mediate on KERNFS_SUICIDAL, you only need to
> worry about this because the mediation is in the first acquisition.  If
> you mediate on KERNFS_SUICIDED, you need to explain that the final
> thing that means the race can't happen is the unbreak in the sysfs
> delete path re-acquiring s_active ... the explanation of what's going
> on and why gets about 2x more complex.

Is it really the dropping of the lock that is causing this?
I don't see that when I read those traces.

I am going to put my vote in for doing all of the self removal
sem-asynchronously by using task_work_add, and killing
device_remove_self, sysfs_remove_self, kernfs_remove_self.  Using
task_work_add remains synchronous with userspace so userspace should not
care, and we get the benefit of not having two different variants of the
same code racing with each other.

It might take a little more work but will leave code that is much more
maintainable in the long run.

Eric