[PATCH RESEND] mdev: fix sysfs traversal with CONFIG_SYSFS_DEPRECATED_V2=y
Joshua Judson Rosen
jrosen at harvestai.com
Mon Aug 3 22:34:52 UTC 2015
Hmm. This is strikingly similar in purpose to the patch that I sent a couple
months ago ("mdev -s: don't mistake inter-device symlinks for separate devices";
using a different approach; it wasn't clear to me whether just disabling
symlink-traversal entirely was the right thing, but skipping the "device"
symlinks did seem to be right even if it was too narrow in scope).
I guess I'm one of the "several people" you mentioned in your follow-up patch :)
On 2015-08-03 17:17, Gregory Fong wrote:
> On Mon, Aug 3, 2015 at 9:07 AM, Denys Vlasenko <vda.linux at googlemail.com> wrote:
>> On Wed, Jul 15, 2015 at 11:10 PM, Gregory Fong <gregory.0xf0 at gmail.com> wrote:
>>> From: Simon Edlund <simon at edlund.nl>
>>>
>>> When mdev -s traverses the /sys directory looking for "dev" files, it
>>> starts with the block devices under /sys/block, and will find the "dev"
>>> file through the symlink, and create a block device node. In the next
>>> stage it will scan the /sys/class looking for char devices, and will
>>> find mtdX/dev again, but this time the mknod will fail because there is
>>> already a device node with that name.
>>
>> By swapping recursive_action("/sys/class") and
>> recursive_action("/sys/block"), now the same thing will happen
>> when the second scan is done.
>>
>> Why this is better?
>
> This is better because if you're using a newer kernel with both
> CONFIG_SYSFS_DEPRECATED=y (and CONFIG_SYSFS_DEPRECATED_V2=y to enable
> it), then where traversing /sys/block with the deprecated layout:
> - /sys/block/mtdblock0 is not a symlink, but an actual directory
> - /sys/block/mtdblock0/device is a symlink to mtd0
>
> IOW, on deprecated sysfs:
> # ls -l /sys/block/mtdblock0
> -r--r--r-- 1 root root 4096 Jan 1 00:00 alignment_offset
> lrwxrwxrwx 1 root root 0 Jan 1 00:00 bdi ->
> ../../devices/virtual/bdi/31:0
> -r--r--r-- 1 root root 4096 Jan 1 00:00 capability
> -r--r--r-- 1 root root 4096 Jan 1 00:00 dev
> lrwxrwxrwx 1 root root 0 Jan 1 00:00 device ->
> ../../devices/platform/brcmnand.0/mtd/mtd0
> -r--r--r-- 1 root root 4096 Jan 1 00:00 discard_alignment
> -r--r--r-- 1 root root 4096 Jan 1 00:00 ext_range
> drwxr-xr-x 2 root root 0 Jan 1 00:00 holders
> -r--r--r-- 1 root root 4096 Jan 1 00:00 inflight
> drwxr-xr-x 2 root root 0 Jan 1 00:00 power
> drwxr-xr-x 3 root root 0 Jan 1 00:00 queue
> -r--r--r-- 1 root root 4096 Jan 1 00:00 range
> -r--r--r-- 1 root root 4096 Jan 1 00:00 removable
> -r--r--r-- 1 root root 4096 Jan 1 00:00 ro
> -r--r--r-- 1 root root 4096 Jan 1 00:00 size
> drwxr-xr-x 2 root root 0 Jan 1 00:00 slaves
> -r--r--r-- 1 root root 4096 Jan 1 00:00 stat
> lrwxrwxrwx 1 root root 0 Jan 1 00:00 subsystem ->
> ../../block
> -rw-r--r-- 1 root root 4096 Jan 1 00:00 uevent
>
> On non-deprecated sysfs:
> # ls -l /sys/block/mtdblock0
> lrwxrwxrwx 1 root root 0 Jan 3 18:57
> /sys/block/mtdblock0 ->
> ../devices/platform/rdb/f03e2800.nand/mtd/mtd0/mtdblock0
>
>
>
> Now, I hadn't really investigated the problem carefully enough until
> now. This patch simply solved a real problem for us when using
> CONFIG_SYSFS_DEPRECATED, and I figured that submitting it would be at
> least a good starting point for someone who actually understands this
> better to figure out a better fix.
>
> But let's see... since recursive_action is called with
> ACTION_FOLLOWLINKS, it will end up checking both
> /sys/block/mtdblock0/dev and /sys/block/mtdblock0/device/dev and
> creating block devices for both of those. That's not correct. So I
> think I now have a real fix here, which is to change the
> recursive_action call that will only be invoked if
> CONFIG_SYSFS_DEPRECATED=y. In that case, we _shouldn't_ need to follow
> symlinks because everything in the top level of /sys/block/ will be a
> real directory, and following them will result in the aforementioned problem.
>
> It was a bit surprising to me that this problem isn't also seen with
> /sys/class/block, since that has the same hierarchy where it could
> follow the link and get to /sys/class/block/mtdblock0/device/dev, but
> looking at recursive_action(), it appears that links are only followed
> where depth == 0, which would explain that.
>
> I'm stuck using the gmail interface right now and so can't copy and
> paste the new patch inline... will send to the list shortly.
>
> Best regards,
> Gregory
>
>>
>>> [gregory: added commit message to patch from BZ #6806]
>>> Signed-off-by: Gregory Fong <gregory.0xf0 at gmail.com>
>>> ---
>>> util-linux/mdev.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/util-linux/mdev.c b/util-linux/mdev.c
>>> index ca4b915..4495b0a 100644
>>> --- a/util-linux/mdev.c
>>> +++ b/util-linux/mdev.c
>>> @@ -1063,6 +1063,9 @@ int mdev_main(int argc UNUSED_PARAM, char **argv)
>>>
>>> putenv((char*)"ACTION=add");
>>>
>>> + recursive_action("/sys/class",
>>> + ACTION_RECURSE | ACTION_FOLLOWLINKS,
>>> + fileAction, dirAction, temp, 0);
>>> /* ACTION_FOLLOWLINKS is needed since in newer kernels
>>> * /sys/block/loop* (for example) are symlinks to dirs,
>>> * not real directories.
>>> @@ -1079,9 +1082,6 @@ int mdev_main(int argc UNUSED_PARAM, char **argv)
>>> ACTION_RECURSE | ACTION_FOLLOWLINKS | ACTION_QUIET,
>>> fileAction, dirAction, temp, 0);
>>> }
>>> - recursive_action("/sys/class",
>>> - ACTION_RECURSE | ACTION_FOLLOWLINKS,
>>> - fileAction, dirAction, temp, 0);
>>> } else {
>>> char *fw;
>>> char *seq;
--
"Don't be afraid to ask (λf.((λx.xx) (λr.f(rr))))."
More information about the busybox
mailing list