[Toybox] [PATCH] df: -a should even show mounts we couldn't stat.
Yi-yo Chiang
yochiang at google.com
Tue Feb 16 12:52:46 PST 2021
There is still one weird case left in df, albeit not related to previous
issues,
$ adb shell df
Filesystem 1K-blocks Used Available Use% Mounted on
/dev/block/dm-1 964296 961376 2920 100% /
tmpfs 2004636 1060 2003576 1% /dev
$ adb shell df /dev/block/dm-1
Filesystem 1K-blocks Used Available Use% Mounted on
tmpfs 2004636 1060 2003576 1% /dev
This is due to the st_dev of /dev/block/dm-1 equals to that of /dev.
However /dev/block/dm-1 is a special device, so df should be checking its
st_rdev, not st_dev.
Only when /dev/block/dm-1 is not mounted, should we check its st_dev... 😵
On Wed, Feb 17, 2021 at 4:27 AM Yi-yo Chiang <yochiang at google.com> wrote:
>
>
> On Wed, Feb 17, 2021 at 12:36 AM Rob Landley <rob at landley.net> wrote:
>
>> On 2/16/21 3:01 AM, Yi-yo Chiang wrote:> > I think the main purpose
>> of the
>> original patch is to show mounts whose
>> > > stat()/statvfs() failed.
>> >
>> > And the current one should do that? (I don't have any mount points
>> I can't stat
>> > because my laptop doesn't do crazy things with linux security
>> modules, maybe I
>> > can put one in a directory I can't read or something to simulate
>> that...)
>> >
>> >
>> > Well the cases that I found actually don't stem from Android security
>> models,
>> > they're actually from the complex bind mounts and over mounts on
>> Android..:( I
>> > simulated the "weird cases" on my linux workstation by:
>> > 1. mkdir -p a/b
>> > 2. mount <tmpfs> on a/b
>> > 3. mount <tmpfs> on a
>> > 4. check output of df [-a] # stat a/b would fail, because (2) is over
>> mounted by (3)
>>
>> Which would only fail if the second tmpfs didn't have a "b" subdirectory.
>> If it
>> did, you'd presumably get a redundant view of the other filesystem?
>>
>> > 5. mkdir a/b
>> > 6. check output of df [-a] # stat a/b would "success", however the
>> st_dev should
>> > be of (3), not (2)
>> > 7. umount a
>> > 8. mount --bind a/b a
>> > 9. etc etc
>>
>> Hmmm... with the debian host df I get:
>>
>> $ sudo mount -t tmpfs uno b/c
>> $ sudo mount -t tmpfs dos b
>> $ mkdir b/c
>> $ df
>> ...
>> dos 8168980 0 8168980 0%
>> /home/landley/toybox/clean2/b
>>
>> It edits "uno" out entirely. (Which is what my old one was doing...)
>>
>> And if I rmdir b/c there's... no change. They're both there in /proc
>> though:
>>
>> uno /home/landley/toybox/clean2/b/c tmpfs rw,relatime 0 0
>> dos /home/landley/toybox/clean2/b tmpfs rw,relatime 0 0
>>
>> Hmmm...
>>
>> Ok, I taught toybox df -a to show those, _and_ used the existing overmount
>> cancel logic to hide the false duplicate when a directory for the mount
>> point
>> exists.
>>
>> Is that good enough or do you want them to show up without -a?
>>
>
> TBH I too am a bit confused with all these testing and comparing output
> back and forth, and I may have confused some commands and stuff :/
> Anyway I think the behaviour now looks good. "df" hides all
> un-stat-able and synth mounts, and "df -a" shows everything in
> /proc/mounts, right?
>
>
>> > I think it does too, but haven't tested it, which is why I asked.
>> >
>> >
>> > Just tested on an Android bench, spotted a few problems:
>> > 1. show_mt is too aggressively filtering out synthetic filesystems.
>> Would
>> > "!mt->statvfs.f_blocks && *mt->device != '/'" be a viable heuristic to
>> filter
>> > out synth fs?
>>
>> How is / a synthetic filesystem? Shouldn't it be initramfs? I've tested
>> that it
>> shows initramfs before... yeah, it's still there in mkroot:
>>
>
> It's a heuristic, I thought devices whose name looks like a path (starts
> with '/') probably isn't a synthetic FS.
> So, !mt->statvfs.f_blocks && *mt->device != '/'
> => device has no block and doesn't start with '/', so assume it's synth-fs
> Looking at your "uno&dos" example above, I feel my heuristic to be very
> fragile. Perhaps a better way is
> !mt->statvfs.f_blocks && !major(mt->stat.st_dev) && mt->stat.st_dev
> => device has no block, major number is 0 and device is not 0:0 (not a
> failed stat()), then assume it's synth-fs
> It doesn't matter much anyways since we don't really care why f_blocks is
> zero (synth-fs or failed stat()), as we are hiding it no matter what.
>
>
>> 8139cp 0000:00:02.0 eth0: link up, 100Mbps, full-duplex, lpa 0x05E1
>> random: fast init done
>> Type exit when done.
>> # df
>> Filesystem 1K-blocks Used Available Use% Mounted on
>> rootfs 29892 656 29236 3% /
>> dev 29892 0 29892 0% /dev
>> #
>>
>> > 2. df still skipping un-stat()-able mounts.
>>
>> Try now?
>>
>> > 3. `df` shows unstatable mounts, however `df <unstatable path/device>`
>> shows
>> > something such as:
>> >
>> > df: '/mnt/pass_through/0/emulated': Permission denied
>> > Filesystem 1K-blocks Used Available Use% Mounted on
>> > df: '/mnt/pass_through/0/emulated': Permission denied
>>
>> Hmmm...
>>
>> $ ./df b/c
>> df: 'b/c': No such file or directory
>> Filesystem 1K-blocks Used Available Use% Mounted on
>> df: 'b/c': No such file or directory
>>
>> Ok, I can at least change that to not give the error message _twice_. But
>> otherwise, at a design level, that's... sort of right? It _isn't_ there.
>>
>> There's a problem where if we can't stat it, we can't tell which one it
>> is out
>> of /proc/mounts because we can't match the device. And you can't do an
>> exact
>> match on the filename because "df ." is a directory under the mount
>> point. I
>> would have to add plumbing to xabspath() and then do a longest match
>> fileunderdir(), which assumes that the /proc/mounts entry is always a
>> cannonical
>> absolute path? (I _think_ that's true in current kernels but would have
>> to read
>> the source)...
>>
>> > this is probably works as intended because the path is unstatable, but
>> we could
>> > do better by string matching the path/device with mount_points/device in
>> > /proc/mounts.
>>
>> We could write a bunch of new code to handle this obscure corner case,
>> yes. But
>> why? We still wouldn't be able to tell anything _about_ the filesystem we
>> can't
>> access, and can only indirectly detect its _existence_.
>>
>>
> I was thinking of a case, where stat(/dev/block/blah) failed but there is
> a mounted entry of /dev/block/blah in /proc/mounts, then we should still
> show the /proc/mounts line because the device name is an exact match.
> But yes I agree with you now, this is still a flawed case because /dev &
> /dev/block/ might be overmounted... We can never be sure what failed to
> stat(/dev/block/blah) actually means, it could mean the user don't have
> enough permission, or /dev/block/blah is hidden to the user etc etc. I
> think having "df -a" to show inaccessible / shadowed mounts is good
> enough.
>
>
> And again, if there _is_ a directory there that's part of an enclosing
>> filesystem, what should we say about it?
>>
>> $ df b/c
>> Filesystem 1K-blocks Used Available Use% Mounted on
>> uno 8168980 0 8168980 0%
>> /home/landley/toybox/clean2/b/c
>> $ ./toybox df b/c
>> Filesystem 1K-blocks Used Available Use% Mounted on
>> dos 8168980 0 8168980 0% /home/landley/toybox/clean2/b
>>
>> The debian one is giving the WRONG ANSWER. (It's dos, not uno.) Mine is
>> giving
>> what filesystem is actually providing that directory, and when there
>> ISN'T such
>> a directory, it says so.
>>
>> Sigh. I suppose I could make it so "df -a dirname" would show all the
>> overmounts? Except that's always going to include rootfs in the list.
>> Right now
>> _exact_ overmounts are filtered out (which is why you don't get rootfs
>> when
>> /dev/sda1 is mounted on / because it's exactly occluded even thought it
>> _is_
>> technically still there).
>>
>>
> Sorry but I don't understand, I thought exact overmounts are shown already?
> $ sudo mount a.img a/b
> $ sudo mount b.img a/b
> $ ./df -a
> /dev/loop0 - - - -
> /ssd2/external/toybox/a/b
> /dev/loop1 2846 45 2521 2%
> /ssd2/external/toybox/a/b
>
>
> This is a design issue. What is the right behavior?
>>
>> > diff --git a/toys/posix/df.c b/toys/posix/df.c
>> > index d8ba2986..a04db1d6 100644
>> > --- a/toys/posix/df.c
>> > +++ b/toys/posix/df.c
>> > @@ -93,7 +93,7 @@ static void show_mt(struct mtab_list *mt, int
>> measuring)
>> > }
>> >
>> > // If we don't have -a, skip synthetic filesystems
>> > - if (!FLAG(a) && !mt->statvfs.f_blocks) return;
>> > + if (!FLAG(a) && !mt->statvfs.f_blocks && *mt->device != '/') return;
>>
>> This means you won't show network mounts or tmpfs instances. (I.E. this
>> would
>> break df so my rootfs example above doesn't show rootfs.)
>>
>> > // Prepare filesystem display fields
>> > *dsuapm = *mt->device == '/' ? xabspath(mt->device, 0) : 0;
>> > @@ -189,7 +190,7 @@ void df_main(void)
>> > // Measure the names then output the table (in filesystem creation
>> order).
>> > for (measuring = 1;;) {
>> > for (mt = mtstart; mt; mt = mt->next)
>> > - if (mt->stat.st_dev) show_mt(mt, measuring);
>> > + show_mt(mt, measuring);
>>
>> I hadn't made it this far down in the email before I already did that. :)
>>
>> > if (!measuring--) break;
>> > print_header();
>> > }
>> >
>> >
>> >
>> > > The only question I have left, is it guaranteed that st_dev must
>> be zero
>> > or left
>> > > unchanged when stat() fails? Or do we need to do something like,
>> "if stat() /
>> > > statvfs() fails, ensure st_dev is zero" in portability.c to
>> ensure the caller
>> > > knows that stat(mount point) failed?
>> >
>> > Linux leaves it alone. I can't speak for Apple.
>> >
>> > That's good enough for me, but I can't say for Apple (I don't own an
>> Apple
>> > station and have never tested on one.)
>>
>> I had one for a while but never got the apple devkit to work because my
>> apple ID
>> glitched basically immediately and an apple guru spent an hour trying to
>> unbork
>> it and couldn't. (I break everything.) Thus I couldn't load xcroak to
>> actually
>> COMPILE anything on said mac. (It then sat on a shelf for 6 months and I
>> eventually mailed it to a college student on twitter who needed a new
>> computer...)
>>
>> Rob
>>
>
>
> --
>
> Yi-yo Chiang
> Software Engineer
> yochiang at google.com
>
--
Yi-yo Chiang
Software Engineer
yochiang at google.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20210217/2c18a6bf/attachment-0001.html>
More information about the Toybox
mailing list