Re: Potential static analysis ideas

["NeilBrown" <neilb@suse.de>]

On Mon, 26 Jul 2021, Arnd Bergmann wrote:
> On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> wrote:
> > On Sun, 25 Jul 2021, Laurent Pinchart wrote:
> > > >
> > > > To make it work well, you need to know if frob() and/or the current
> > > > function return an error code or not.  While you can use some heuristics
> > > > (e.g. is there any return -Exxx), perhaps we can add an annotation to
> > > > indicate if a function returns an error code, or an error pointer?
> > >
> > > https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/
> > >
> > > I think it would be useful, if not for the tools, at least for
> > > developers.
> >
> > Agreed.  I added some code to smatch so that I could annotate pointers to
> > say if they are allowed to be NULL.  The implementation isn't perfect,
> > but I love having that extra documentation about when I do or don't have
> > to check for NULL.
> 
> I can think of four different annotations that limit what a pointer return from
> a function can be:
> 
> a) either a valid pointer or NULL, but never an error pointer,
> b) either a valid pointer or an error pointer, but not NULL,
> c) always a valid pointer, never NULL or an error,
> d) always NULL, but callers are expected to check for error pointers.
> 
> The last one is the tricky bit because there are a couple of subsystems
> that intentionally return an opaque NULL pointer to indicate success
> when the subsystem is compile-time disabled, and expect callers to not
> dereference that pointer but instead pass it into other stubbed-out
> interfaces. I think this one is rather specific to the kernel, but also really
> useful to have in a static checker because everyone gets those wrong.

And now we see the worms wriggling their way out of the can....

Some pointers can also have the 'lsb' overloaded, sometimes as a
bit-spin-lock.
And pointers stored in a structure can have temporal type requirements -
different at various places in the life cycle.
Most obviously, "non-NULL" pointers can be NULL (or unitiallised) during
inititalization, and may have other inconsistencies during finalization.
But it isn't hard to find more complexity.

"context" structures are particularly suseptible to this.  I've been
looking at 'struct fs_context' recently.  Normally you would use one of
the allocation functions which will always set ->ops and a few other
fields. So maybe ->ops is "always valid".
Then you initialize some extra bits.  Maybe call vfs_parse_fs_string()
etc.
Finally you call vfs_create_mount().  But you *can* call
vfs_create_mount() after only setting ->root ->sb_flags and ->source.
->ops can be uninitialised!

So what annotations would you give to the various fields? To capture
this completely you would need a series of sub-types of 'fs_context',
each with different guarantees or requirements about the values of
different fields.

I love contemplating what the "perfect" system would be, but I doubt a
near-perfect system would be much fun to program in, and that last 20%
to get to perfection would take a while.

So if we wanted to do something like this, it would need to be
completely opt-in.  The default would be current behaviour, and we would
want to have annotations for both what is, and what isn't, promised.

"what is" would be something like "this pointer is never NULL".
"what isn't" would be more like __must_check.  "this pointer might be
NULL and you really have to check before you dereference it".
For an unannotated pointer, it is not an error to test if it is NULL, or
to dereference without testing.

And, of course, we would need to be able to cast between the different
promises.

NeilBrown