lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 3 Oct 2021 22:57:58 -0700 (PDT) From: Stefan Schaeckeler <schaecsn@....net> To: richard@....at CC: linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org, sschaeck@...co.com Subject: Re: [PATCH 1/1] ubifs: ubifs to export filesystem error counters Hello Richard, > > Not all ubifs filesystem errors are propagated to userspace. > > > > Export bad magic, bad node and crc errors via sysfs. This allows userspace > > to notice filesystem errors: > > > > /sys/fs/ubifs/ubiX_Y/errors_magic > > /sys/fs/ubifs/ubiX_Y/errors_node > > /sys/fs/ubifs/ubiX_Y/errors_crc > > > > The counters are reset to 0 with a remount. Writing anything into the > > counters also clears them. > > I think this is a nice feature. Thanks for implementing it. > Please see some minor nits below. > > Is there a specific reason why you didn't implement it via UBIFS's debugfs interface? These error counters are not meant for aiding debugging but for userspace to monitor the sanity of the filesystem. Also ext4 exports error counters via sysfs - it's probably a good idea to be consistent with them. $ dir /sys/fs/ext4/sdb2/*error* -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/errors_count -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/first_error_block -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/first_error_errcode -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/first_error_func -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/first_error_ino -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/first_error_line -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/first_error_time -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/last_error_block -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/last_error_errcode -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/last_error_func -r--r--r-- 1 root root 4096 Oct 3 13:47 /sys/fs/ext4/sdb2/last_error_ino -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/last_error_line -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/last_error_time --w------- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/trigger_fs_error > sysfs is ABI, so we cannot change much anymore. Judging by the filesystem permissions above, ext4 does not seem to allow resetting error counters. If you worry about unchangable ABIs then we could play it safe and don't support write callbacks for resetting the error counters in an initial version of the ubifs sysfs. What do you think? When we are at it, in the current code, writing anything into a sysfs node resets the corresponding counter. One could fine-tune that to set the counter to whatever non-negative integer is passed. > > + if (c->stats) > > + c->stats->magic_errors++; > > Please wrap this into a helper function. Ack. > > + if (c->stats) > > + c->stats->node_errors++; > > Same. Ack. > > + if (c->stats) > > + c->stats->crc_errors++; > > Same. Ack. > > +#define UBIFS_ATTR_FUNC(_name, _mode) UBIFS_ATTR(_name, _mode, _name) > > + > > +UBIFS_ATTR_FUNC(errors_magic, 0644); > > +UBIFS_ATTR_FUNC(errors_crc, 0644); > > +UBIFS_ATTR_FUNC(errors_node, 0644); > > I'm not sure whether everyone should be allowed to read these stats. > dmesg is also restricted these days. An unprivileged user should not see the > errors he can indirectly trigger. I don't mind 600, but having error counters world-readable is consistent with ext4 (see dir above) and so I suggest to keep 644. > > + case attr_errors_crc: > > + return snprintf(buf, PAGE_SIZE, "%u\n", > > + sbi->stats->crc_errors); > > Please use sysfs_emit(). Ack. > > + if (n == UBIFS_DFS_DIR_LEN) { > > + /* The array size is too small */ > > + ret = -EINVAL; > > + goto out_last; > > Where is c->stats released in case of an error? My fault. Will be fixed. > > diff --git a/fs/ubifs/sysfs.h b/fs/ubifs/sysfs.h > > new file mode 100644 > > index 000000000000..a10a82585af8 > > --- /dev/null > > +++ b/fs/ubifs/sysfs.h > > Do we really need a new header file? > Usually most run-time stuff of UBIFS is part of ubifs.h. I wanted to be consistent with debugfs where fs/ubifs/debug.c comes with its own header fs/ubifs/debug.h. I'll send out a new patch once we agree on all changes. To recap: - write callbacks: do we remove them? If not, do we keep them as is or do we fine-tine them by letting a non-negative integer set the counter? - 644 (world-readable) counters to be consistent with ext4? - keep sysfs.h to be consistent with debugfs? Stefan
Powered by blists - more mailing lists