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@zeniv.linux.org.uk 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/5f960cb10f56ec2017c128ef9d16060e...
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 withthe 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 doingthe 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@arm.com Date: Wed Aug 15 10:55:44 2018 +0100
print address space number for explicit cast to ulongThis 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@arm.com
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);elsewarning(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