On Tue, 2016-11-08 at 13:13 -0600, Eric W. Biederman wrote:
> James Bottomley <jejb@xxxxxxxxxxxxxxxxxx> 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.
No, it's an ABBA lock inversion that causes this. The traces are
somewhat dense, but they say it here:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(s_active#336);
lock(&shost->scan_mutex);
lock(s_active#336);
lock(&shost->scan_mutex);
*** DEADLOCK ***
The detailed explanation of this is here:
http://marc.info/?l=linux-scsi&m=147855187425596
The fix is ensuring that the CPU1 thread doesn't get into taking
s_active if CPU0 already has it using the KERNFS_SUICIDED/AL flag as an
indicator.
James
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
