GNU bug report logs - #21372
df prioritizes some bind mounts over real ones.

Previous Next

Full log

Message #29 received at 21372 <at> debbugs.gnu.org (full text, mbox):

From: Dave Chiluk <dave.chiluk <at> canonical.com>
To: Pádraig Brady <P <at> draigBrady.com>, 
 Dave Chiluk <chiluk <at> canonical.com>, 21372 <at> debbugs.gnu.org
Subject: Re: bug#21372: [PATCH] df: fix prioritize real mounts over bind mounts
Date: Fri, 11 Sep 2015 16:55:07 -0500

On 09/11/2015 03:42 PM, Pádraig Brady wrote:
> On 31/08/15 22:05, Dave Chiluk wrote:
>> Fixes an issue where bind mounts with shorter mount directories than the
>> original mount are prioritized when running df.  The root cause of this
>> is that /proc/self/mountinfo now lists the filesystem device with bind
>> mounts rather than the source directory. With /etc/mtab the source
>> device was listed as the originating directory so this was not an issue.
>>
>> More information is available here.
>> https://bugs.launchpad.net/ubuntu/+source/coreutils/+bug/1432871
>> ---
>>  src/df.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/df.c b/src/df.c
>> index 2e541b9..00c77c1 100644
>> --- a/src/df.c
>> +++ b/src/df.c
>> @@ -652,9 +652,14 @@ filter_mount_list (bool devices_only)
>>                else if ((strchr (me->me_devname, '/')
>>                         /* let "real" devices with '/' in the name win.  */
>>                          && ! strchr (devlist->me->me_devname, '/'))
>> -                       /* let a shorter mountdir win.  */
>> -                       || (strlen (devlist->me->me_mountdir)
>> +                       /* let a shorter mountdir win. */
>> +                       /* Only if it's not a bind mount. */
>> +                       || ((strlen (devlist->me->me_mountdir)
>>                             > strlen (me->me_mountdir))
>> +                          && (devlist->me->me_mntroot != NULL
>> +                             && me->me_mntroot != NULL
>> +                             && (strlen (devlist->me->me_mntroot)
>> +                                 > strlen(me->me_mntroot))))
> 
> As previously mentioned, should this be >= ?
Sorry my apologies for not addressing that.  I don't think it should be
(strlen (devlist->me->me_mntroot)>= strlen(me->me_mntroot)), because
that would give preference to later mounts of the same me_mntroot
directory *(I'm specifically thinking of the case where both me_mntroot
= "/").

However, looking at the code again, I realized that by adding the NULL
checks I have actually changed the behavior for non-linux OS's since the
NULL checks would always defeat the (strlen (devlist->me->me_mountdir) >
strlen (me->me_mountdir) check.

I'm going to think through this nasty if case and see if I can make it
prettier.

> 
> We'll need to look at tests.
> This will be quite tricky given the current
> avoidance of /proc/self/mountinfo in tests.
> I'll look at adding a separate test for this.
> 
> thanks,
> Pádraig.
> 

Additionally I agree with you that we should mirror the find_mnt
bracketting behavior.
    /dev/sdb2       xfs              12.7G  9.4G   3.3G  74% /
    /dev/sdb2[/etc] xfs                                     /root/chroot

I'll submit that in a different patch once we have this more squared away.

Thanks,
Dave.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.