Hi,
I just spent some time trying to convert some so far PPC-only drivers to
be more generic. One of the things I had to do was convert stuff like
u32 *val = of_get_property(np, "bla", NULL);
do_stuff_with(*val);
with
of_property_read_u32(np, "bla", &val);
do_stuff_with(val);
(error checking omitted for simplicity). The problem is that
of_get_property() just returns void*. When the property is just a
string, there's no problem interpreting that as a char*. But when the
property is a number of array of numbers, I'd like some way to flag
casting it to u32* as an error - if you cast it to a (pointer to integer
type wider than char), it must be to a __be32*. Is there some way
sparse/smatch could help find such cases?
Rasmus
On Mon, Oct 28, 2019 at 08:32:42PM +0100, Rasmus Villemoes wrote:
> Hi,
>
> I just spent some time trying to convert some so far PPC-only drivers to
> be more generic. One of the things I had to do was convert stuff like
>
> u32 *val = of_get_property(np, "bla", NULL);
> do_stuff_with(*val);
>
> with
>
> of_property_read_u32(np, "bla", &val);
> do_stuff_with(val);
>
> (error checking omitted for simplicity). The problem is that
> of_get_property() just returns void*. When the property is just a
> string, there's no problem interpreting that as a char*. But when the
> property is a number of array of numbers, I'd like some way to flag
> casting it to u32* as an error - if you cast it to a (pointer to integer
> type wider than char), it must be to a __be32*. Is there some way
> sparse/smatch could help find such cases?
If I understand you correctly, you would need a kind of 'soft'
bitwise pointer?
I guess it shouldn't be too hard to add a new flag which would
allow cast of bitwise pointers to pointers to char/void (see
at end of evaluate.c:evaluate_cast()).
Note: casts from bitwise pointer to void* are already allowed.
-- Luc
On 28/10/2019 23.49, Luc Van Oostenryck wrote:
> On Mon, Oct 28, 2019 at 08:32:42PM +0100, Rasmus Villemoes wrote:
>> Hi,
>>
>> I just spent some time trying to convert some so far PPC-only drivers to
>> be more generic. One of the things I had to do was convert stuff like
>>
>> u32 *val = of_get_property(np, "bla", NULL);
>> do_stuff_with(*val);
>>
>> with
>>
>> of_property_read_u32(np, "bla", &val);
>> do_stuff_with(val);
>>
>> (error checking omitted for simplicity). The problem is that
>> of_get_property() just returns void*. When the property is just a
>> string, there's no problem interpreting that as a char*. But when the
>> property is a number of array of numbers, I'd like some way to flag
>> casting it to u32* as an error - if you cast it to a (pointer to integer
>> type wider than char), it must be to a __be32*. Is there some way
>> sparse/smatch could help find such cases?
>
> If I understand you correctly, you would need a kind of 'soft'
> bitwise pointer?
Yes, that's a very good way of putting it.
> I guess it shouldn't be too hard to add a new flag which would
> allow cast of bitwise pointers to pointers to char/void (see
> at end of evaluate.c:evaluate_cast()).
Hm, yeah, but it should also allow casting to __be32* , but not u32* or
__le32* (though somebody must have gone out of their way to introduce a
bug in the latter case). Don't spend too much time on it, I was just
wondering if there was an easy (maybe already existing) way.
Thanks,
Rasmus
This should probably work?
regards,
dan carpenter
diff --git a/check_list.h b/check_list.h
index 564bd1c2..5fa9a269 100644
--- a/check_list.h
+++ b/check_list.h
@@ -191,6 +191,7 @@ CK(check_nospec_barrier)
CK(check_spectre)
CK(check_spectre_second_half)
CK(check_implicit_dependencies)
+CK(check_of_get_property)
/* wine specific stuff */
CK(check_wine_filehandles)
diff --git a/check_of_get_property.c b/check_of_get_property.c
new file mode 100644
index 00000000..b7aa8824
--- /dev/null
+++ b/check_of_get_property.c
@@ -0,0 +1,47 @@
+/*
+ * Copyright (C) 2019 Oracle.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
+ */
+
+#include "smatch.h"
+#include "smatch_slist.h"
+
+static int my_id;
+
+static void match_of_get_property(const char *fn, struct expression *expr, void *unused)
+{
+ struct expression *left = expr->left;
+ struct symbol *type;
+
+ type = get_type(left);
+ if (!type || type->type != SYM_PTR)
+ return;
+ type = get_base_type(type);
+ if (type_bits(type) == 8)
+ return;
+ if (type->type == SYM_RESTRICT)
+ return;
+ sm_warning("'%s' returns big endian data", fn);
+}
+
+void check_of_get_property(int id)
+{
+ my_id = id;
+
+ if (option_project != PROJ_KERNEL)
+ return;
+
+ add_function_assign_hook("of_get_property", &match_of_get_property, NULL);
+}
On 29/10/2019 11.50, Dan Carpenter wrote:
> This should probably work?
I haven't tested it, but yes, something like that. Can you also do the
case of struct property::value, i.e. handling
struct property *p = ...;
u32 *v = p->value;
Thanks,
Rasmus
On Tue, Oct 29, 2019 at 01:50:58PM +0300, Dan Carpenter wrote:
> +static void match_of_get_property(const char *fn, struct expression *expr, void *unused)
> +{
> + struct expression *left = expr->left;
> + struct symbol *type;
> +
> + type = get_type(left);
> + if (!type || type->type != SYM_PTR)
> + return;
> + type = get_base_type(type);
> + if (type_bits(type) == 8)
> + return;
> + if (type->type == SYM_RESTRICT)
> + return;
Wouldn't this also silently accept assignments to any bitwise
type: __le32, __be16, ... ?
-- Luc
On Tue, Oct 29, 2019 at 12:43:24PM +0100, Rasmus Villemoes wrote:
> On 29/10/2019 11.50, Dan Carpenter wrote:
> > This should probably work?
>
> I haven't tested it, but yes, something like that. Can you also do the
> case of struct property::value, i.e. handling
>
> struct property *p = ...;
> u32 *v = p->value;
Attached.
This has a bunch of flaws like "void *value = p->value;" doesn't
generate a warning. A function that returns a u32 pointer doing
"return p->value;" doesn't generate a value.
Anyway, it's a starting point to experiment with.
regards,
dan carpenter
On Tue, Oct 29, 2019 at 12:47:50PM +0100, Luc Van Oostenryck wrote:
> On Tue, Oct 29, 2019 at 01:50:58PM +0300, Dan Carpenter wrote:
> > +static void match_of_get_property(const char *fn, struct expression *expr, void *unused)
> > +{
> > + struct expression *left = expr->left;
> > + struct symbol *type;
> > +
> > + type = get_type(left);
> > + if (!type || type->type != SYM_PTR)
> > + return;
> > + type = get_base_type(type);
> > + if (type_bits(type) == 8)
> > + return;
> > + if (type->type == SYM_RESTRICT)
> > + return;
>
> Wouldn't this also silently accept assignments to any bitwise
> type: __le32, __be16, ... ?
It does, yes. I'm not sure how big of an issue that is... I always
just throw a check together and test it before I decide if it's worth
investing more time into it.
regards,
dan carpenter
On Tue, Oct 29, 2019 at 03:55:19PM +0300, Dan Carpenter wrote:
> On Tue, Oct 29, 2019 at 12:47:50PM +0100, Luc Van Oostenryck wrote:
> > On Tue, Oct 29, 2019 at 01:50:58PM +0300, Dan Carpenter wrote:
> > > +static void match_of_get_property(const char *fn, struct expression *expr, void *unused)
> > > +{
> > > + struct expression *left = expr->left;
> > > + struct symbol *type;
> > > +
> > > + type = get_type(left);
> > > + if (!type || type->type != SYM_PTR)
> > > + return;
> > > + type = get_base_type(type);
> > > + if (type_bits(type) == 8)
> > > + return;
> > > + if (type->type == SYM_RESTRICT)
> > > + return;
> >
> > Wouldn't this also silently accept assignments to any bitwise
> > type: __le32, __be16, ... ?
>
> It does, yes. I'm not sure how big of an issue that is...
Probably not much if it's just a one shot for Rasmus and
probably not much more otherwise.
> I always
> just throw a check together and test it before I decide if it's worth
> investing more time into it.
Sure, but I was thinking about false negatives here.
Regards,
-- Luc