From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f69.google.com (mail-wm0-f69.google.com [74.125.82.69]) by kanga.kvack.org (Postfix) with ESMTP id 4BF2F6B74A6 for ; Wed, 5 Sep 2018 15:03:22 -0400 (EDT) Received: by mail-wm0-f69.google.com with SMTP id c14-v6so5896788wmb.2 for ; Wed, 05 Sep 2018 12:03:22 -0700 (PDT) Received: from mail-sor-f65.google.com (mail-sor-f65.google.com. [209.85.220.65]) by mx.google.com with SMTPS id r196-v6sor2171257wmg.11.2018.09.05.12.03.20 for (Google Transport Security); Wed, 05 Sep 2018 12:03:20 -0700 (PDT) Date: Wed, 5 Sep 2018 21:03:17 +0200 From: Luc Van Oostenryck Subject: Re: [PATCH v6 11/11] arm64: annotate user pointers casts detected by sparse Message-ID: <20180905190316.a34yycthgbamx2t3@ltop.local> References: <5d54526e5ff2e5ad63d0dfdd9ab17cf359afa4f2.1535629099.git.andreyknvl@google.com> <20180831081123.6mo62xnk54pvlxmc@ltop.local> <20180831134244.GB19965@ZenIV.linux.org.uk> <01cadefd-c929-cb45-500d-7043cf3943f6@arm.com> <20180903151026.n2jak3e4yqusnogt@ltop.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: owner-linux-mm@kvack.org List-ID: To: Vincenzo Frascino Cc: Andrey Konovalov , Al Viro , Mark Rutland , Kate Stewart , linux-doc@vger.kernel.org, Catalin Marinas , Will Deacon , Kostya Serebryany , linux-kselftest@vger.kernel.org, Chintan Pandya , Shuah Khan , Ingo Molnar , linux-arch@vger.kernel.org, Jacob Bramley , Linux ARM , Evgeniy Stepanov , Kees Cook , Ruben Ayrapetyan , Ramana Radhakrishnan , Dmitry Vyukov , Linux Memory Management List , Greg Kroah-Hartman , LKML , Lee Smith , Andrew Morton , Robin Murphy , "Kirill A . Shutemov" 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 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 > 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 > > 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 ()", sas); > + else > + warning(expr->pos, "cast removes address space of expression"); > + } I think that the if (Wcast_from_as) is unneeded, the can be added even if Wcast_from_as is false. Woukd it be OK for you? -- Luc