On Tue, Oct 05, 2021 at 09:30:10AM -0700, Kees Cook wrote: > On Mon, Sep 27, 2021 at 09:37:56AM -0700, Luis Chamberlain wrote: > > --- /dev/null > > +++ b/lib/test_sysfs.c > > @@ -0,0 +1,921 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1 > > +/* > > + * sysfs test driver > > + * > > + * Copyright (C) 2021 Luis Chamberlain <mcgrof@xxxxxxxxxx> > > + * > > + * This program is free software; you can redistribute it and/or modify it > > + * under the terms of the GNU General Public License as published by the Free > > + * Software Foundation; either version 2 of the License, or at your option any > > + * later version; or, when distributed separately from the Linux kernel or > > + * when incorporated into other software packages, subject to the following > > + * license: > > + * > > + * This program is free software; you can redistribute it and/or modify it > > + * under the terms of copyleft-next (version 0.3.1 or later) as published > > + * at http://copyleft-next.org/. > > As Greg suggested, please drop the boilerplate here. Sure, sorry for missing that fixed. > > +static ssize_t config_show(struct device *dev, > > + struct device_attribute *attr, > > + char *buf) > > +{ > > + struct sysfs_test_device *test_dev = dev_to_test_dev(dev); > > + struct test_config *config = &test_dev->config; > > + int len = 0; > > + > > + test_dev_config_lock(test_dev); > > + > > + len += snprintf(buf, PAGE_SIZE, > > + "Configuration for: %s\n", > > + dev_name(dev)); > > Please use sysfs_emit() instead of snprintf(). Oh nice, done and fixed also in the other places. > > +static int sysfs_test_dev_alloc_blockdev(struct sysfs_test_device *test_dev) > > +{ > > + int ret = -ENOMEM; > > + > > + test_dev->disk = blk_alloc_disk(NUMA_NO_NODE); > > + if (!test_dev->disk) { > > + pr_err("Error allocating disk structure for device %d\n", > > + test_dev->dev_idx); > > + goto out; > > + } > > + > > + test_dev->disk->major = sysfs_test_major; > > + test_dev->disk->first_minor = test_dev->dev_idx + 1; > > + test_dev->disk->fops = &sysfs_testdev_ops; > > + test_dev->disk->private_data = test_dev; > > + snprintf(test_dev->disk->disk_name, 16, "test_sysfs%d", > > + test_dev->dev_idx); > > Prefer sizeof(test_dev->disk->disk_name) over open-coded "16". Sure. > > +static ssize_t read_reset_first_test_dev(struct file *file, > > + char __user *user_buf, > > + size_t count, loff_t *ppos) > > +{ > > + ssize_t len; > > + char buf[32]; > > + > > + reset_first_test_dev++; > > + len = sprintf(buf, "%d\n", reset_first_test_dev); > > Even though it's safe as-is, I was going to suggest scnprintf() here > (i.e. explicit bounds and a bounds-checked "len"). However, scnprintf() > returns ssize_t, and there's no bounds checking in > simple_read_from_buffer. That needs fixing (I'll send a patch). OK we can later change it to scnprintf() once your patch gets merged. > > --- /dev/null > > +++ b/tools/testing/selftests/sysfs/sysfs.sh > > @@ -0,0 +1,1208 @@ > > +#!/bin/bash > > +# SPDX-License-Identifier: GPL-2.0-or-later > > +# Copyright (C) 2021 Luis Chamberlain <mcgrof@xxxxxxxxxx> > > +# > > +# This program is free software; you can redistribute it and/or modify it > > +# under the terms of the GNU General Public License as published by the Free > > +# Software Foundation; either version 2 of the License, or at your option any > > +# later version; or, when distributed separately from the Linux kernel or > > +# when incorporated into other software packages, subject to the following > > +# license: > > +# > > +# This program is free software; you can redistribute it and/or modify it > > +# under the terms of copyleft-next (version 0.3.1 or later) as published > > +# at http://copyleft-next.org/. > > + > > +# This performs a series tests against the sysfs filesystem. > > -boilerplate Nuked. > > +check_dmesg() > > +{ > > + # filter out intentional WARNINGs or Oopses > > + local filter=${1:-_check_dmesg_filter} > > + > > + _dmesg_since_test_start | $filter >$seqres.dmesg > > + egrep -q -e "kernel BUG at" \ > > + -e "WARNING:" \ > > + -e "\bBUG:" \ > > + -e "Oops:" \ > > + -e "possible recursive locking detected" \ > > + -e "Internal error" \ > > + -e "(INFO|ERR): suspicious RCU usage" \ > > + -e "INFO: possible circular locking dependency detected" \ > > + -e "general protection fault:" \ > > + -e "BUG .* remaining" \ > > + -e "UBSAN:" \ > > + $seqres.dmesg > > Is just looking for "call trace" sufficient here? So far from my testing yes. This strategy is also borrowed from fstests and that's what is done there, and so quite a lot of testing has been done with that. If we are to consider an enhancement here we should then also consider an enhancement welcome for fstests. Luis
