[Toybox] [PATCH] df: -a should even show mounts we couldn't stat.
Yi-yo Chiang
yochiang at google.com
Tue Feb 16 12:27:18 PST 2021
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20210217/9366f106/attachment-0001.html>
More information about the Toybox
mailing list