Re: [PATCH v6 11/11] arm64: annotate user pointers casts detected by sparse

[Luc Van Oostenryck]

On Tue, Sep 04, 2018 at 12:27:23PM +0100, Vincenzo Frascino wrote:
> On 03/09/18 16:10, Luc Van Oostenryck wrote:
> > On Mon, Sep 03, 2018 at 02:49:38PM +0100, Vincenzo Frascino wrote:
> >> On 03/09/18 13:34, Andrey Konovalov wrote:
> >>> On Fri, Aug 31, 2018 at 3:42 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> >>>> On Fri, Aug 31, 2018 at 10:11:24AM +0200, Luc Van Oostenryck wrote:
> >>>>> On Thu, Aug 30, 2018 at 01:41:16PM +0200, Andrey Konovalov wrote:
> >>>>>> This patch adds __force annotations for __user pointers casts detected by
> >>>>>> sparse with the -Wcast-from-as flag enabled (added in [1]).
> >>>>>>
> >>>>>> [1] https://github.com/lucvoo/sparse-dev/commit/5f960cb10f56ec2017c128ef9d16060e0145f292
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> It would be nice to have some explanation for why these added __force
> >>>>> are useful.
> >>>
> >>> I'll add this in the next version, thanks!
> >>>
> >>>>         It would be even more useful if that series would either deal with
> >>>> the noise for real ("that's what we intend here, that's what we intend there,
> >>>> here's a primitive for such-and-such kind of cases, here we actually
> >>>> ought to pass __user pointer instead of unsigned long", etc.) or left it
> >>>> unmasked.
> >>>>
> >>>>         As it is, __force says only one thing: "I know the code is doing
> >>>> the right thing here".  That belongs in primitives, and I do *not* mean the
> >>>> #define cast_to_ulong(x) ((__force unsigned long)(x))
> >>>> kind.
> >>>>
> >>>>         Folks, if you don't want to deal with that - leave the warnings be.
> >>>> They do carry more information than "someone has slapped __force in that place".
> >>>>
> >>>> Al, very annoyed by that kind of information-hiding crap...
> >>>
> >>> This patch only adds __force to hide the reports I've looked at and
> >>> decided that the code does the right thing. The cases where this is
> >>> not the case are handled by the previous patches in the patchset. I'll
> >>> this to the patch description as well. Is that OK?
> >>>
> >> I think as well that we should make explicit the information that
> >> __force is hiding.
> >> A possible solution could be defining some new address spaces and use
> >> them where it is relevant in the kernel. Something like:
> >>
> >> # define __compat_ptr __attribute__((noderef, address_space(5)))
> >> # define __tagged_ptr __attribute__((noderef, address_space(6)))
> >>
> >> In this way sparse can still identify the casting and trigger a warning.
> >>
> >> We could at that point modify sparse to ignore these conversions when a
> >> specific flag is passed (i.e. -Wignore-compat-ptr, -Wignore-tagged-ptr)
> >> to exclude from the generated warnings the ones we have already dealt
> >> with.
> >>
> >> What do you think about this approach?
> > 
> > I'll be happy to add such warnings to sparse if it is useful to detect
> > (and correct!) problems. I'm also thinking to other possiblities, like
> > having some weaker form of __force (maybe simply __force_as (which will
> > 'only' force the address space) or even __force_as(TO, FROM) (with TO
> > and FROM being a mask of the address space allowed).I believe we need something here to address this type of problems and I like
> your proposal of adding a weaker force in the form of __force_as(TO, FROM)
> because I think it provides the right level information. 
> 
> > However, for the specific situation here, I'm not sure that using
> > address spaces is the right choice because I suspect that the concept
> > of tagged pointer is orthogonal to the one of (the usual) address space
> > (it won't be possible for a pointer to be __tagged_ptr *and* __user).
> I was thinking to address spaces because the information seems easily accessible
> in sparse [1], but I am certainly open to any solution that can be semantically
> more correct.

Yes, adding a new address space is easy (and doesn't need any modification
to sparse). Here, I think adding a new 'modifier' __tagged (much like
__nocast, __noderef, ...) would be much more appropriate.
I think that at this point, it would be nice to have a clear description
of the problem and what sort of checks are wanted.
 
> > 
> > OTOH, when I see already the tons of warnings for concepts established
> > since many years (I'm thinking especially at __bitwise, see [1]) I'm a
> > bit affraid of adding new, more specialized ones that people will
> > understand even less how/when they need to use them.
> Thanks for providing this statistic, it is very interesting. I understand your
> concern, but I think that in this case we need a more specialized option not only
> to find potential problems but even to provide the right amount of information
> to who reads the code. 
> 
> A solution could be to let __force_as(TO, FROM) behave like __force and silence
> the warning by default, but have an option in sparse to re-enable it 
> (i.e. -Wshow-force-as). 

That would be, indeed, a simple solution but IMO even more dangerous than
__force itself (since by readingthe code with this annotation  people would 
naturally think it only involves the AS will in fact it would be the same
as __force). I prefer to directly implement a plain __force_as, forcing
only the AS).
 
> [1]
> ---
> commit ee7985f0c2b29c96aefe78df4139209eb4e719d8
> Author: Vincenzo Frascino <vincenzo.frascino@xxxxxxx>
> Date:   Wed Aug 15 10:55:44 2018 +0100
> 
>     print address space number for explicit cast to ulong
>     
>     This patch build on top of commit b34880d ("stricter warning
>     for explicit cast to ulong") and prints the address space
>     number when a "warning: cast removes address space of expression"
>     is triggered.
>     
>     This makes easier to discriminate in between different address
>     spaces.
>     
>     A validation example is provided as well as part of this patch.
>     
>     Signed-off-by: Vincenzo Frascino <vincenzo.frascino@xxxxxxx>
> 
> diff --git a/evaluate.c b/evaluate.c
> index 6d5d479..2fc0ebc 100644
> --- a/evaluate.c
> +++ b/evaluate.c
> @@ -3017,8 +3017,12 @@ static struct symbol *evaluate_cast(struct expression *expr)
>  		sas = stype->ctype.as;
>  	}
>  
> -	if (!tas && sas > 0)
> -		warning(expr->pos, "cast removes address space of expression");
> +	if (!tas && sas > 0) {
> +		if (Wcast_from_as)
> +			warning(expr->pos, "cast removes address space of expression (<asn:%d>)", sas);
> +		else
> +			warning(expr->pos, "cast removes address space of expression");
> +	}

I think that the if (Wcast_from_as) is unneeded, the <asn:%d> can be added
even if Wcast_from_as is false. Woukd it be OK for you?

-- Luc