OpenWrt/LEDE Project

  • Status Waiting on reporter
  • Percent Complete
    0%
  • Task Type Bug Report
  • Category Base system
  • Assigned To No-one
  • Operating System All
  • Severity Medium
  • Priority Very Low
  • Reported Version All
  • Due in Version Undecided
  • Due Date Undecided
  • Private
Attached to Project: OpenWrt/LEDE Project
Opened by Sebastian - 21.03.2019

FS#2196 - block info confuses mount points

I’ve encountered a bug in /sbin/block.

If multiple block devices paths share the same prefix e.g. /dev/mmcblk0p1 and /dev/mmcblk0p10 the commands block info confuses them.
In my case /dev/mmcblk0p1 is a boot partition and /dev/mmcblk0p10 is a data partition.
There are a bunch of other partitions, which I’ll omit for the sake clarity in the following example.

/etc/config/fstab:

config 'global'
        option  anon_swap       '0'
        option  anon_mount      '0'
        option  auto_swap       '1'
        option  auto_mount      '1'
        option  delay_root      '5'
        option  check_fs        '0'

config 'mount'
        option  target  '/data'
        option  device  '/dev/mmcblk0p10'
        option  fstype  'ext4'
        option  enabled '1'

config 'mount'
        option  device  '/dev/mmcblk0p1'
        option  target  '/uboot'
        option  fstype  'vfat'
        option  options 'rw,sync'
        option  enabled '1'
...

After boot the output of mount looks like this:

root@foobar:~# mount
/dev/root on /rom type squashfs (ro,relatime)
proc on /proc type proc (rw,nosuid,nodev,noexec,noatime)
sysfs on /sys type sysfs (rw,nosuid,nodev,noexec,noatime)
tmpfs on /tmp type tmpfs (rw,nosuid,nodev,noatime)
tmpfs on /dev type tmpfs (rw,nosuid,relatime,size=512k,mode=755)
devpts on /dev/pts type devpts (rw,nosuid,noexec,relatime,mode=600,ptmxmode=000)
debugfs on /sys/kernel/debug type debugfs (rw,noatime)
/dev/mmcblk0p10 on /data type ext4 (rw,relatime,data=ordered)
/dev/mmcblk0p1 on /uboot type vfat (rw,sync,relatime,fmask=0000,dmask=0000,allow_utime=0022,codepage=437,iocharset=iso8859-1,shortname=mixed,errors=remount-ro)
...

At least everything is mounted in the right place.
If I do a block info I get:

root@foobar:~# block info
/dev/mmcblk0p1: UUID="0037-0032" LABEL="" VERSION="FAT16" MOUNT="/data" TYPE="vfat"
/dev/mmcblk0p10: UUID="d8dff841-edc4-4094-ae68-f6f0596c2277" VERSION="1.0" MOUNT="/data" TYPE="ext4"
...

Which is a little bit odd, as /dev/mmcblk0p1 is clearly not mounted under /data.

I did some digging in the sources and found the culprit in block.c:find_mount_point.
In line 646 in block.c:

*pos = '\0';
devname = tmp;
if (!strncmp(block, devname, len)) { // <- problem
	point = strdup(cpoint);
	break;
}

block is the parameter passed to find_mount_point, len is the string length of block and devname is a block device name read from /proc/self/mountinfo.
If the string in block is a prefix of the string in devname, !strncmp will always be true as, therefore the mount point in this line will be returned to caller.
In my case the line for /dev/mmcblk0p10 is encountered first, therefore /data is returned as mountpoint for both /dev/mmcblk0p10 and /dev/mmcblk0p1.

Since the idea here is to look for an exactly equal string I would suggest to compare the string length as well as calling strncmp.
See the attached patch for my temporary solution.

As a last remark: I could not determine if this affects other functionalities of /sbin/block as well,
but since find_mount_point is also called during umounting this might be more than just a display bug.

Sebastian commented on 06.06.2019 14:23

Since this issue has not be updated, I'll do it myself.

There seem to be similar string matching bugs in other parts of the codebase.
The following two commit contain the necessary changes:

https://git.openwrt.org/?p=project/fstools.git;a=commitdiff;h=3957dd39c6c2413c4341258d63a913948f99ac6f

https://git.openwrt.org/?p=project/fstools.git;a=commitdiff;h=9b36dc25dd31197681dfcbe4e83879a9b885f451

As personal side note: I don't think replacing all strncmps calls with strcmp is the best choice.

Admin
Jo-Philipp Wich commented on 06.06.2019 14:57

I didn't simply replace all strncmp() calls with strcmp() ones but analyzed the surrounding code context to see whether it makes sense or not.

For the changed places where strncmp() was used, it was not useful, inappropriate or offered no advantage over a simple strcmp().

  • strncmp(block, devname, len) (with len = strlen(block)) wrongly matches substrings since it does not compare trailing \0
  • strncmp(block, devname, len + 1) (with len = strlen(block)) is identical to strcmp(block, devname) but more expensive due to the required strlen() call
  • strncmp(name, volname, strlen(volname) + 1) is equivalent to strcmp(name, volname)

I would agree that changing strncmp() to strcmp() is a problem when the length argument is not derived from strlen() but for the cases that have been changed I do not see the problem.

What would have been the best choice in your opinion?

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing