race between util_create_path() and util_delete_path()?

race between util_create_path() and util_delete_path()?

[Florian Zumbiehl]

Hi,

as it's quite a bit of effort to actually verify this without much
knowlegde about the structure of udev: Could it happen that
util_create_path() and util_delete_path() do run in parallel for
the same directory? After all, util_create_path() does handle
the case where creation of the directory happens in parallel
to it running, so it doesn't seem all that unlikely to me ...

Florian

Re: race between util_create_path() and util_delete_path()?

[Alan Jenkins]

On 8/29/09, Florian Zumbiehl <florz@florz.de> wrote:
> Hi,
>
> as it's quite a bit of effort to actually verify this without much
> knowlegde about the structure of udev: Could it happen that
> util_create_path() and util_delete_path() do run in parallel for
> the same directory? After all, util_create_path() does handle
> the case where creation of the directory happens in parallel
> to it running, so it doesn't seem all that unlikely to me ...
>
> Florian

Events on the same device are serialized.  So you never have an "add"
event running in parallel with a "remove" event for the same device.
That covers most cases, but there might be an exception.

Consider two CHANGE events running in parallel.  Changes to symlinks
are recorded under /dev/.udev/names.  If the CHANGE events cause one
symlink to be removed, and a new symlink to be created with the same
name, I think you would get simultaneous calls to delete_path() and
create_path().

Alan

Re: race between util_create_path() and util_delete_path()?

[Florian Zumbiehl]

Hi,

> On 8/29/09, Florian Zumbiehl <florz@florz.de> wrote:
> > as it's quite a bit of effort to actually verify this without much
> > knowlegde about the structure of udev: Could it happen that
> > util_create_path() and util_delete_path() do run in parallel for
> > the same directory? After all, util_create_path() does handle
> > the case where creation of the directory happens in parallel
> > to it running, so it doesn't seem all that unlikely to me ...
> 
> Events on the same device are serialized.  So you never have an "add"

what is the "same device" identified by?

> event running in parallel with a "remove" event for the same device.
> That covers most cases, but there might be an exception.
> 
> Consider two CHANGE events running in parallel.  Changes to symlinks
> are recorded under /dev/.udev/names.  If the CHANGE events cause one
> symlink to be removed, and a new symlink to be created with the same
> name, I think you would get simultaneous calls to delete_path() and
> create_path().

I was actually thinking about the much simpler case where there could be
two completely independent objects that happen to reside in the same
directory, or at least below a common path prefix, and one of them is
being removed while the other one is being created.

Well, I guess that that's one of those bugs I don't have enough insight
into udev for fixing, as there probably is no local fix that just touches
a few lines within one function - so, any suggestions as to how to proceed?

Florian

Re: race between util_create_path() and util_delete_path()?

[Alan Jenkins]

On 8/30/09, Florian Zumbiehl <florz@florz.de> wrote:
> Hi,
>
>> On 8/29/09, Florian Zumbiehl <florz@florz.de> wrote:
>> > as it's quite a bit of effort to actually verify this without much
>> > knowlegde about the structure of udev: Could it happen that
>> > util_create_path() and util_delete_path() do run in parallel for
>> > the same directory? After all, util_create_path() does handle
>> > the case where creation of the directory happens in parallel
>> > to it running, so it doesn't seem all that unlikely to me ...
>>
>> Events on the same device are serialized.  So you never have an "add"
>
> what is the "same device" identified by?

I was thinking of the sysfs path.  See event_queue_manager() and
devpath_busy() in udevd.c.

It looks like (minor:major, subsystem) is also checked; I don't know why.

>> event running in parallel with a "remove" event for the same device.
>> That covers most cases, but there might be an exception.
>>
>> Consider two CHANGE events running in parallel.  Changes to symlinks
>> are recorded under /dev/.udev/names.  If the CHANGE events cause one
>> symlink to be removed, and a new symlink to be created with the same
>> name, I think you would get simultaneous calls to delete_path() and
>> create_path().
>
> I was actually thinking about the much simpler case where there could be
> two completely independent objects that happen to reside in the same
> directory, or at least below a common path prefix, and one of them is
> being removed while the other one is being created.

Wups.  I thought that case couldn't happen for some reason, and then
found the more complex case which is really just the same problem.  So
yes, I think this can happen.

> Well, I guess that that's one of those bugs I don't have enough insight
> into udev for fixing, as there probably is no local fix that just touches
> a few lines within one function - so, any suggestions as to how to proceed?

Hmm. This is the problem, right?

1 mkdir /dev/foo -> EEXIST
 2 unlink /dev/foo/bar
 2 rmdir /dev/foo -> success
1 mknod /dev/foo/qux -> ENOENT

I think the solution is to audit the callers of create_path(), and fix
them to retry indefinitely if the subsequent mknod / creat fails with
ENOENT.

Alan

Re: race between util_create_path() and util_delete_path()?

[Kay Sievers]

On Mon, Aug 31, 2009 at 11:22, Alan
Jenkins<sourcejedi.lkml@googlemail.com> wrote:
> On 8/30/09, Florian Zumbiehl <florz@florz.de> wrote:
>>> On 8/29/09, Florian Zumbiehl <florz@florz.de> wrote:
>>> > as it's quite a bit of effort to actually verify this without much
>>> > knowlegde about the structure of udev: Could it happen that
>>> > util_create_path() and util_delete_path() do run in parallel for
>>> > the same directory? After all, util_create_path() does handle
>>> > the case where creation of the directory happens in parallel
>>> > to it running, so it doesn't seem all that unlikely to me ...
>>>
>>> Events on the same device are serialized.  So you never have an "add"
>>
>> what is the "same device" identified by?
>
> I was thinking of the sysfs path.  See event_queue_manager() and
> devpath_busy() in udevd.c.
>
> It looks like (minor:major, subsystem) is also checked; I don't know why.

That was for renamed/moved devices which are the same but have a
different devpath. We have DEVPATH_OLD since a while, and the
minor:major check is gone, right?

Kay

Re: race between util_create_path() and util_delete_path()?

[Alan Jenkins]

On 8/31/09, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Mon, Aug 31, 2009 at 11:22, Alan
> Jenkins<sourcejedi.lkml@googlemail.com> wrote:
>> On 8/30/09, Florian Zumbiehl <florz@florz.de> wrote:
>>>> On 8/29/09, Florian Zumbiehl <florz@florz.de> wrote:
>>>> > as it's quite a bit of effort to actually verify this without much
>>>> > knowlegde about the structure of udev: Could it happen that
>>>> > util_create_path() and util_delete_path() do run in parallel for
>>>> > the same directory? After all, util_create_path() does handle
>>>> > the case where creation of the directory happens in parallel
>>>> > to it running, so it doesn't seem all that unlikely to me ...
>>>>
>>>> Events on the same device are serialized.  So you never have an "add"
>>>
>>> what is the "same device" identified by?
>>
>> I was thinking of the sysfs path.  See event_queue_manager() and
>> devpath_busy() in udevd.c.
>>
>> It looks like (minor:major, subsystem) is also checked; I don't know why.
>
> That was for renamed/moved devices which are the same but have a
> different devpath. We have DEVPATH_OLD since a while, and the
> minor:major check is gone, right?

You're right, I was reading the source from an old checkout.  I was
puzzled by the subsytem check... but that would have been necessary
for block v.s. char devices.  Nevermind.

Regards
Alan

Re: race between util_create_path() and util_delete_path()?

[Florian Zumbiehl]

Hi,

> > Well, I guess that that's one of those bugs I don't have enough insight
> > into udev for fixing, as there probably is no local fix that just touches
> > a few lines within one function - so, any suggestions as to how to proceed?
> 
> Hmm. This is the problem, right?
> 
> 1 mkdir /dev/foo -> EEXIST
>  2 unlink /dev/foo/bar
>  2 rmdir /dev/foo -> success
> 1 mknod /dev/foo/qux -> ENOENT

I guess so :-)

> I think the solution is to audit the callers of create_path(), and fix
> them to retry indefinitely if the subsequent mknod / creat fails with
> ENOENT.

Seems like a rather dirty solution to me - what about serializing any
directory creation/removal plus subsequent object creation using flock()
on a lock file? Would still need changes at every call site, of course ...

Florian

Re: race between util_create_path() and util_delete_path()?

[Alan Jenkins]

On 8/31/09, Florian Zumbiehl <florz@florz.de> wrote:
> Hi,
>
>> > Well, I guess that that's one of those bugs I don't have enough insight
>> > into udev for fixing, as there probably is no local fix that just
>> > touches
>> > a few lines within one function - so, any suggestions as to how to
>> > proceed?
>>
>> Hmm. This is the problem, right?
>>
>> 1 mkdir /dev/foo -> EEXIST
>>  2 unlink /dev/foo/bar
>>  2 rmdir /dev/foo -> success
>> 1 mknod /dev/foo/qux -> ENOENT
>
> I guess so :-)
>
>> I think the solution is to audit the callers of create_path(), and fix
>> them to retry indefinitely if the subsequent mknod / creat fails with
>> ENOENT.
>
> Seems like a rather dirty solution to me - what about serializing any
> directory creation/removal plus subsequent object creation using flock()
> on a lock file? Would still need changes at every call site, of course ...

That would limit paralellism to some degree.

Retrys won't be needed very often; they would surely have a lower
overhead.  Less code too :-).  It's not completely out of left field;
both seqlocks and transactional memory use retries.  There's no risk
of livelock (i.e. infinite retries)...

I'm lazy and don't want udev to run measurably slower.  The only
advantage I see of flock() is that it tells the reader "I'm a
synchronization primitive, solving a concurrency problem here".  I
think we can get close enough by explaining the race in a comment next
to the definition of create_path().

Alan