[Toybox] [PATCH] df: -a should even show mounts we couldn't stat.

[Yi-yo Chiang]

On Mon, Feb 15, 2021 at 10:31 PM Rob Landley <rob at landley.net> wrote:

> Sigh, thunderbird crashed and I lost 8 gazillion open reply windows with
> half-composed messages. (Kmail used to remember messages being composed
> and open
> them back up again when it was relaunched. I miss kmail, but they tied it
> to a
> boat anchor of a desktop...)
>
> Oh well, makes closing down my laptop so I can do a distro version upgrade
> only
> a half day instead of a full day's work...
>
> On 2/14/21 1:32 PM, Yi-yo Chiang wrote:
> > Hi Rob,
> >
> > 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)
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



> > For example, an under-privileged user may be able to read /proc/mounts
> but lack
> > the permission to stat(vfs) the mount point, so showing "-" is a way of
> saying
> > "I know this XXX device is being mount on YYY mount point, however for
> whatever
> > reason I lack the means to get usage information from the filesystem".
>
> And the test I'm using is that the dev node is zero, which should never
> happen
> because you can't mount a NULL device. Instead of adding an external flag,
> it's
> the same in-band signaling it was using before.
>
> Previously, the logic would skip such mounts (you could get them in the
> list of
> mount points if all you wanted was the path, but we had no info about them
> so df
> would skip them). Elliott apparently wanted to display them as  - - - -
> entries
> and I _think_ the current code does that but haven't tested it because I
> can
> stat all my filesystems because I'm not crazy enough to use LFS.
>
> > So I think 5f5f97f215bb accomplishes what the original change wants by
> not
> > skipping 0:0 device at all and gives "st_dev == 0" a special meaning of
> > "stat(mount point) failed".
>
> 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?
2. df still skipping un-stat()-able mounts.
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

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.

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;

   // 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);
       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.)


> Rob
>

Thanks!
-- 

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/20210216/e6af0904/attachment-0001.htm>