Re: Proposal: validate FFS root inode during the mount.

[Maciej <maciej.grochowski%pm.me@localhost>]

Apologizes for bit vague description, I will try to describe it better
We touch a couple of different topics so I will address then in order.

1. My biggest concern, and reason for putting this validation in place,
is the case where we allow mount the Filesystem that later cannot be umounted from Userland.

Both umount(2) and umount(8) (with or without -R) will fail as they relay on kernel vfs_lookup in the path resolution step.
When root inode does not have i_mode set we will end up with ENOENT due to the failure inside ffs_loadvnode
```
ffs_loadvnode()

        if (ip->i_mode == 0) {
                ffs_deinit_vnode(ump, vp);

                return ENOENT;
        }
```
All lookup operations end up in vcache -> VFS_LOADVNODE, so is clear that we won't be able to resolve the given path without loading inode from disk.

From a user perspective, this can lead to the situations where after corruption user cannot umount FS and the only way is to power off system in order to umount it and repair,
which IMO is bad/unexpected behaviour.
Although i_mode should not be zero, the strange value inside i_mode can lead to different execution paths, because of that I want to check if the i_mode is a directory.

Hope people will share a similar opinion about this part.


2. other validation for root inode,

` !S_ISDIR(dp1->di_mode) || !dp1->di_size || !dp1->di_blocks || !dp1->di_nlink) `

Couple other cases that I found probably worth to consider for FFS root inode and were placed inside the patch,
Note: I DID NOT prove that incorrect values inside them case the panic during the mount(2) system call step, but only when another system call is issued.

di_blocks - if is equal to zero lstat operations will cause the panic, that may happen due to the tools on the startup. But is only MIGHT so someone may argue that this can happen to any other file and we clearly do not want to do a scan on every mount.

di_nlink - if equal to the zero and we have di_blocks or di_size non-equal to the zero, vput operation will cause panic inside ufs_inactive. That can happen due to i.e. stat systemcall, so most likely during the boot proces.

That is my justification about rest of validation which can be considered as pragmatic/unnecessary but as I already checked DIR mode on the root inode, checking these fields does not cost anything.
Also I did not check all cases and is possible that some strange combination can cause panic on the mount.
Another argument about this validation is the fact that is done inside ffs_mount which is also called by the reload which can cause panic due to the usage of vput.
As I said unfortunately at this moment I don't have particular crash example, so I understand people concerns.

3. "Kirk has added a bunch of checksum and other integrity checks to FreeBSD$"

 Thank you for the info I will spend some time and study what is validated there.

4. Validation on the mount time: I don't have a strong opinion about the preferred behaviour and also do not want to check entire FS on the mount because that will make mount process very unuseful.

To summarize:
I consider point 1 (umount issue) as a logical bug fix, and 2 as nice to have (or ew possible future fix).

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Wednesday, November 20, 2019 8:27 PM, <mlelstv%serpens.de@localhost> wrote:

> mouse%Rodents-Montreal.ORG@localhost (Mouse) writes:
>
> > But for the machine's own filesystems? Corruption should panic:
> > mount -o onerror=panic /dev/wd2a /builds
>
> There are 3 popular behaviours and all can be useful.
>
> -   return errors
> -   return errors + reject further writes
> -   panic
>
>     The second is Linux default behaviour and some people hate it,
>     maybe because the read-only mode is rather silent. If you do
>     react to it, it can be more graceful than a panic.
>
>     --
>     --
>     Michael van Elst
>     Internet: mlelstv%serpens.de@localhost
>     "A potential Snark may lurk in every tree."
>