WTF: driver-core-next contains recursive directory removal!

WTF: driver-core-next contains recursive directory removal!

[Eric W. Biederman]

Greg what is  going on?  I just looked and Tejuns ill-conceived recursive
directory deletion code has been merged into your driver-core-next tree.

That code is semantically broken.  I reviewed it and I gave the reasons
why it was wrong.  You came up to me and mentioned at LPC that you
agreed with my reasons.  And yet I just looked in driver-core-next and
there the code is in all of it's broken glory.

Please pull out that crap it has no business ever going into a stable
kernel.

The short version is unless someone has drastically changed pci hotplug
since last time I looked the code is dramatically wrong as pci hotplug
removes directories in the wrong order remove the parent first.

So now we have sysfs code that is remove directories multiple times, and
we just finished the conversation about sysfs_assoc_lock where Tejun was
just explaining how multiple removal of kobjects is not safe.

This sysfs semantic change is an unmaintainable disaster.

Linus if you would be so kind as to not merge this disaster when the
merge window opens I would very much appreciate it.

Eric

Re: WTF: driver-core-next contains recursive directory removal!

[Greg Kroah-Hartman]

On Wed, Oct 30, 2013 at 03:38:58PM -0700, Eric W. Biederman wrote:
> 
> Greg what is  going on?  I just looked and Tejuns ill-conceived recursive
> directory deletion code has been merged into your driver-core-next tree.
> 
> That code is semantically broken.  I reviewed it and I gave the reasons
> why it was wrong.  You came up to me and mentioned at LPC that you
> agreed with my reasons.  And yet I just looked in driver-core-next and
> there the code is in all of it's broken glory.

Because I tested it out, and there were no such problems.

> Please pull out that crap it has no business ever going into a stable
> kernel.

Really?  It seems to survive my testing here just fine.

> The short version is unless someone has drastically changed pci hotplug
> since last time I looked the code is dramatically wrong as pci hotplug
> removes directories in the wrong order remove the parent first.

That should still work here, and I'm pretty sure I tested it, but will
do so before I send it to Linus.

I don't think there's an issue here, otherwise both Tejun and I would
have found some issues during testing, same for all of the other
linux-next users for the past few weeks.

thanks,

greg k-h

Re: WTF: driver-core-next contains recursive directory removal!

[Eric W. Biederman]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> I don't think there's an issue here, otherwise both Tejun and I would
> have found some issues during testing, same for all of the other
> linux-next users for the past few weeks.

There issues were subtle and hard to detect especially without
instrumenting the code during pci hotplug to look for them.  Memory
leaks, use after free, and needing pci hotplug to reproduce them made
the kinds of bugs I saw when I was working with it easy to go unnoticed
in light testing.

Beyond that the code has the deep issue that the code breaks normal
filesystem expectations in a way that is certain to confuse filesystem
people like Al Viro.

And yes that code being at all recursive is one of the things that Viro
objected to when you had him review sysfs before merging my cleanups
long ago.

Recursive removal is absolutely unnecessary, and it hides bugs, and
makes the code unnecessarily complex for no good reason.

Eric

Re: WTF: driver-core-next contains recursive directory removal!

[Tejun Heo]

(cc'ing Al, hi!)

Hey, again.

On Wed, Oct 30, 2013 at 03:57:37PM -0700, Eric W. Biederman wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> > I don't think there's an issue here, otherwise both Tejun and I would
> > have found some issues during testing, same for all of the other
> > linux-next users for the past few weeks.
> 
> There issues were subtle and hard to detect especially without
> instrumenting the code during pci hotplug to look for them.  Memory
> leaks, use after free, and needing pci hotplug to reproduce them made
> the kinds of bugs I saw when I was working with it easy to go unnoticed
> in light testing.

You're missing the part where kobject holds an extra reference of its
sysfs_dirent.  Regardless of the removal order, kobj->sd doesn't go
away until the kobj is explicitly destroyed.  I have no idea how that
would lead to the various subtle and critical issues you claim to
exist.  Can you please elaborate what you're thinking about?  As it
currently stands, it's severely lacking details.

> Beyond that the code has the deep issue that the code breaks normal
> filesystem expectations in a way that is certain to confuse filesystem
> people like Al Viro.

First of all, the current sysfs behavior is likely more confusing than
the new one - we delete local files but don't recurse into
subdirectories.  This used to work fine when all sysfs directories
were associated with a kobj as we could simply defer sysfs lifetime
management to that of kobjs.  This no longer is true.  We have
subdirectories which aren't associated with kobjects and our removal
policy is strange at best - files which exist immediately below a kobj
directory are recursively removed automatically but files under
subdirectories and subdirectories themselves aren't.  If you forget to
delete them, it doesn't even fail.  They just leak.

We can go either direction - either make removal properly recursive or
require manual recursion from sysfs users, and the merged patches
implement the former.  While the latter *could* be an option too, I
don't see how that is a good idea.  We'd be requiring meaningless
boilerplate cleanup code for no good reason and when a subsystem
forgets to delete an odd file, the options we could take would be
either 1. fail directory deletion or 2. leak undeleted nodes.  Both
suck.  Anything which complicates and fails in exit path is a pretty
bad design.  What is a driver to do after its "kill me" call fails?

I asked you multiple times why you keep mentioning Al Viro.  IIRC,
there was an issue that Al ran into back when sysfs was using vfs
constructs for its internal representation, which hasn't been the case
for ages now.  vfs interacts with sysfs the same way it interacts with
any other distributed file systems.  I hope you're not asserting that
recursive removal from remote is somehow confusing to vfs.

The implemented behavior is something which is fully understood and
can be described extremely concisely - the removal is recusrive.  I
don't think many people would have problem grasping that.

> And yes that code being at all recursive is one of the things that Viro
> objected to when you had him review sysfs before merging my cleanups
> long ago.

IIRC, my first major involvement with sysfs was completing conversion
to using sysfs_dirent exclusively for its internal representation and
I don't recall "your cleanups".  Maybe it was before my involvement.
I don't see why Al would have problem with sysfs implementing
recursive removal of its nodes, tho.  The argument is strange because
our current behavior is something a lot more unexpected.

Al, can you please chime in if you still care?

> Recursive removal is absolutely unnecessary, and it hides bugs, and
> makes the code unnecessarily complex for no good reason.

So, are you saying that it's better to require each and every user of
sysfs to remove all files and subdirectories on their cleanup paths?
Why is that beneficial?  Wouldn't that be a lot more code than
necessary?  What does that buy us?

Thanks.

-- 
tejun

Re: WTF: driver-core-next contains recursive directory removal!

[Eric W. Biederman]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Wed, Oct 30, 2013 at 03:38:58PM -0700, Eric W. Biederman wrote:
>> 
>> Greg what is  going on?  I just looked and Tejuns ill-conceived recursive
>> directory deletion code has been merged into your driver-core-next tree.
>> 
>> That code is semantically broken.  I reviewed it and I gave the reasons
>> why it was wrong.  You came up to me and mentioned at LPC that you
>> agreed with my reasons.  And yet I just looked in driver-core-next and
>> there the code is in all of it's broken glory.
>
> Because I tested it out, and there were no such problems.

The biggest and worst issue is the semantics are total unmaintable
garbage and there has never been a single counter argument to that.

Recursive delete is WRONG.

Further there was no follow up converstion on the list about this trash
to say you had tested it.  Or anything else.

Even the partial recursive delete we currently have has been responsible
for broken users of sysfs so I don't see how adding additional checks is
going to do anything but paper over real bugs in real uses of sysfs.

Will you please remove that garbage from your tree. 

Eric

Re: WTF: driver-core-next contains recursive directory removal!

[Tejun Heo]

Hello, Eric.

On Wed, Oct 30, 2013 at 06:19:27PM -0700, Eric W. Biederman wrote:
> The biggest and worst issue is the semantics are total unmaintable
> garbage and there has never been a single counter argument to that.
> 
> Recursive delete is WRONG.
> 
> Further there was no follow up converstion on the list about this trash
> to say you had tested it.  Or anything else.

I think you're going over the line.  You were cc'd on all patches and
all the counter points that I wrote in this thread are repetitions
from the previous threads.  I don't remember you answering to any of
the counter points.

> Even the partial recursive delete we currently have has been responsible
> for broken users of sysfs so I don't see how adding additional checks is
> going to do anything but paper over real bugs in real uses of sysfs.

Can you please go into details of what kind of bugs would be
introduced by recursive removal?  I'd really like to know and be happy
to fix any actual issues but you really aren't providing useful enough
information.

> Will you please remove that garbage from your tree. 

Eric, chill.

-- 
tejun

Re: WTF: driver-core-next contains recursive directory removal!

[Bjorn Helgaas]

[+cc linux-pci]

On Wed, Oct 30, 2013 at 4:38 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> Greg what is  going on?  I just looked and Tejuns ill-conceived recursive
> directory deletion code has been merged into your driver-core-next tree.
>
> That code is semantically broken.  I reviewed it and I gave the reasons
> why it was wrong.  You came up to me and mentioned at LPC that you
> agreed with my reasons.  And yet I just looked in driver-core-next and
> there the code is in all of it's broken glory.
>
> Please pull out that crap it has no business ever going into a stable
> kernel.
>
> The short version is unless someone has drastically changed pci hotplug
> since last time I looked the code is dramatically wrong as pci hotplug
> removes directories in the wrong order remove the parent first.

Can you please elaborate on exactly where PCI hotplug removes things
in the wrong order?  We are struggling with many issues in PCI
removal, and if you can help us fix them by pointing out an issue,
that would be great.

I did work through removal of a small tree [1], and it appeared to me
that we removed sysfs things in bottom-up order, but maybe I missed
something or you're talking about a different situation.

Thanks,
  Bjorn

[1] http://lkml.kernel.org/r/CAErSpo6g5vR0NMs18YZBz+6RGZd0zNeDu_=1Pk4c5a+OqHfS1g@mail.gmail.com

> So now we have sysfs code that is remove directories multiple times, and
> we just finished the conversation about sysfs_assoc_lock where Tejun was
> just explaining how multiple removal of kobjects is not safe.
>
> This sysfs semantic change is an unmaintainable disaster.
>
> Linus if you would be so kind as to not merge this disaster when the
> merge window opens I would very much appreciate it.
>
> Eric
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: WTF: driver-core-next contains recursive directory removal!

[Tejun Heo]

Hey, Eric.

On Wed, Oct 30, 2013 at 03:38:58PM -0700, Eric W. Biederman wrote:
> Please pull out that crap it has no business ever going into a stable
> kernel.

Can you please calm down a bit?  It's becoming pretty difficult to
take your opinions as technical especially as all the points you're
raising had already been discussed before without you providing much
meat to it.  I'll just go over all the points in the thread and
reiterate what was discussed before just in case.

> The short version is unless someone has drastically changed pci hotplug
> since last time I looked the code is dramatically wrong as pci hotplug
> removes directories in the wrong order remove the parent first.

We already discussed this.  kobject sysfs interface code holds onto
its sysfs_dirent until the kobject itself is removed.  Given that the
existing sysfs interface doesn't even provide sd based interface for
files or subdirectories (all removals are by dir + name), this
shouldn't break anything.  Even if removal order is reversed, the only
behavior change would be that a kobj would be disconnected from the
hierarchy if one of parent is removed before it.  I can't think of a
scenario where that'd lead to an explosion.  Even in the very unlikely
case this causes an actual problem, I can't see how this would be
something too difficult to rectify.

> So now we have sysfs code that is remove directories multiple times, and
> we just finished the conversation about sysfs_assoc_lock where Tejun was
> just explaining how multiple removal of kobjects is not safe.

No, you're completely misunderstanding what I said in the thread.
Multiple *concurrent* removal invocations on the same kobject is
unsafe (no intrinsic reason for it to be that way tho) becaues of the
way kobj <-> sysfs association is handled - it's about synchronizing
conflicting operations to the same sysfs_diernt.  The sysfs hierarchy
proper has always been fully protected by sysfs_mutex.  There's
nothing racy about recursive removal.  In fact, we've been doing
partial recursive removal for a very long time now.

> This sysfs semantic change is an unmaintainable disaster.

That's a very strong statement without matching backing arguments.
AFAICS, most, if not all, of your arguments stem either from
misunderstanding or heavily handwavy.  I'll continue on the thread.

> Linus if you would be so kind as to not merge this disaster when the
> merge window opens I would very much appreciate it.

Nice going.

-- 
tejun