Re: [PATCH 1/3] lib, include/linux: add usercopy failure capability

[Marco Elver]

On Fri, Aug 21, 2020 at 01:51PM +0200, Dmitry Vyukov wrote:
...
> > +++ b/lib/fault-inject-usercopy.c
> > @@ -0,0 +1,66 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +#include <linux/fault-inject.h>
> > +#include <linux/fault-inject-usercopy.h>
> > +#include <linux/random.h>
> > +
> > +static struct {
> > +       struct fault_attr attr;
> > +       u32 failsize;
> > +} fail_usercopy = {
> > +       .attr = FAULT_ATTR_INITIALIZER,
> > +       .failsize = 0,
> > +};
> > +
> > +static int __init setup_fail_usercopy(char *str)
> > +{
> > +       return setup_fault_attr(&fail_usercopy.attr, str);
> > +}
> > +__setup("fail_usercopy=", setup_fail_usercopy);
> > +
> > +#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
> > +
> > +static int __init fail_usercopy_debugfs(void)
> > +{
> > +       umode_t mode = S_IFREG | 0600;
> > +       struct dentry *dir;
> > +
> > +       dir = fault_create_debugfs_attr("fail_usercopy", NULL,
> > +                                       &fail_usercopy.attr);
> > +       if (IS_ERR(dir))
> > +               return PTR_ERR(dir);
> > +
> > +       debugfs_create_u32("failsize", mode, dir,
> > +                          &fail_usercopy.failsize);
> 
> Marco, what's the right way to annotate these concurrent accesses for KCSAN?

For debugfs variables that are accessed concurrently, the only
non-data-racy option (currently) is to use debugfs_create_atomic_t() and
make the variable an atomic_t.

If it's read-mostly as is the case here, and given that atomic_read() is
cheap (it maps to READ_ONCE on x86 and arm64), that'd be reasonable even
if performance is a concern.

> > +       return 0;
> > +}
> > +
> > +late_initcall(fail_usercopy_debugfs);
> > +
> > +#endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */
> > +
> > +/**
> > + * should_fail_usercopy() - Failure code or amount of bytes not to copy.
> > + * @n: Size of the original copy call.
> > + *
> > + * The general idea is to have a method which returns the amount of bytes not
> > + * to copy, a failure to return, or 0 if the calling function should progress
> > + * without a failure. E.g., copy_{to,from}_user should NOT copy the amount of
> > + * bytes returned by should_fail_usercopy, returning this value (in addition
> > + * to any bytes that could actually not be copied) or a failure.
> > + *
> > + * Return: one of:
> > + * negative, failure to return;
> > + * 0, progress normally;
> > + * a number in ]0, n], the number of bytes not to copy.
> > + *
> > + */
> > +long should_fail_usercopy(unsigned long n)
> > +{
> > +       if (should_fail(&fail_usercopy.attr, n)) {
> > +               if (fail_usercopy.failsize > 0)
> > +                       return fail_usercopy.failsize % (n + 1);

If you wanted to retain the u32 in debugfs, you can mark this
'data_race(fail_usercopy.failsize)' -- since what we're doing here is
probabilistic anyway, reading a garbage value won't affect things much.

Alternatively, just switch to atomic_t and it'll just be an
atomic_read().

Thanks,
-- Marco