Re: [PATCH net-next v12 1/4] net netlink: Add new type NLA_BITFIELD32

[Jiri Pirko]

Mon, Jul 31, 2017 at 02:03:55PM CEST, j...@mojatatu.com wrote:
>On 17-07-31 02:38 AM, Jiri Pirko wrote:
>> Sun, Jul 30, 2017 at 09:59:10PM CEST, j...@mojatatu.com wrote:
>> > Jiri,
>> > 
>> > This is getting exhausting, seriously.
>> > I posted the code you are commenting one two days ago so i dont have to
>> > repost.
>> 
>> And I commented on the "*u32 = *u32" thing. But you ignored it. Pardon
>> me for mentioning that again now :/
>> 
>
>You commented on *u32 assignment from *void which i fixed. I
>intentionally selected the different assignment names to reflect
>meaning. Had you commented earlier - although I would have found

Yep, I don't understand why the function arg cannot have the desired
name right away. Also, I don't understand why you don't just have u32
instead of pointer as a local variable, if you really needed this local
variable. Ok, I admit that ":)" is probably not intuitive comment.
Will be more blunt next time.


>it disagreable - I would have fixed that too. Jiri, you need to be
>more tolerant so progress can be made at times.

I don't think so. I believe that it is really important that code can be
read nicely. If we don't do it, it will be just mess (like it is in lot
of net/sched/ places).


>
>> 
>> > 
>> > On D. Ahern: I dont think we are disagreeing anymore on the need to
>> > generalize the check. He is saying it should be a helper and I already
>> > had the validation data; either works. I dont see the gapping need
>> > to remove the validation data.
>> 
>> DavidA? Your opinion.
>> 
>
>With DavidA(reading his response) - the issue is one of taste.
>Again either approach is fine. You can call helpers for every user
>or make them invoked behind the scenes.
>Again - like all your comments on code taste which I addressed, I
>would have made that change if the comment had come in earlier. I got
>exhausted. Imagine how a newbie corporate guy wouldve felt after this.

That's how it is. 

Re: [PATCH net-next v12 1/4] net netlink: Add new type NLA_BITFIELD32

[Jamal Hadi Salim]

On 17-07-31 02:38 AM, Jiri Pirko wrote:

Sun, Jul 30, 2017 at 09:59:10PM CEST, j...@mojatatu.com wrote:

Jiri,

This is getting exhausting, seriously.
I posted the code you are commenting one two days ago so i dont have to
repost.


And I commented on the "*u32 = *u32" thing. But you ignored it. Pardon
me for mentioning that again now :/



You commented on *u32 assignment from *void which i fixed. I
intentionally selected the different assignment names to reflect
meaning. Had you commented earlier - although I would have found
it disagreable - I would have fixed that too. Jiri, you need to be
more tolerant so progress can be made at times.





On D. Ahern: I dont think we are disagreeing anymore on the need to
generalize the check. He is saying it should be a helper and I already
had the validation data; either works. I dont see the gapping need
to remove the validation data.


DavidA? Your opinion.



With DavidA(reading his response) - the issue is one of taste.
Again either approach is fine. You can call helpers for every user
or make them invoked behind the scenes.
Again - like all your comments on code taste which I addressed, I
would have made that change if the comment had come in earlier. I got
exhausted. Imagine how a newbie corporate guy wouldve felt after this.

cheers,
jamal

Re: [PATCH net-next v12 1/4] net netlink: Add new type NLA_BITFIELD32

[Jiri Pirko]

Sun, Jul 30, 2017 at 09:59:10PM CEST, j...@mojatatu.com wrote:
>Jiri,
>
>This is getting exhausting, seriously.
>I posted the code you are commenting one two days ago so i dont have to
>repost.

And I commented on the "*u32 = *u32" thing. But you ignored it. Pardon
me for mentioning that again now :/


>
>On D. Ahern: I dont think we are disagreeing anymore on the need to
>generalize the check. He is saying it should be a helper and I already
>had the validation data; either works. I dont see the gapping need
>to remove the validation data.

DavidA? Your opinion.

Re: [PATCH net-next v12 1/4] net netlink: Add new type NLA_BITFIELD32

[David Ahern]

On 7/30/17 1:59 PM, Jamal Hadi Salim wrote:
> On D. Ahern: I dont think we are disagreeing anymore on the need to
> generalize the check. He is saying it should be a helper and I already
> had the validation data; either works. I dont see the gapping need
> to remove the validation data.

I never disagreed on general code; I have always disagreed on validating
values as part of the policy check.

Re: [PATCH net-next v12 1/4] net netlink: Add new type NLA_BITFIELD32

[Jamal Hadi Salim]

Jiri,

This is getting exhausting, seriously.
I posted the code you are commenting one two days ago so i dont have to
repost.

On D. Ahern: I dont think we are disagreeing anymore on the need to
generalize the check. He is saying it should be a helper and I already
had the validation data; either works. I dont see the gapping need
to remove the validation data.

cheers,
jamal

On 17-07-30 02:42 PM, Jiri Pirko wrote:

Sun, Jul 30, 2017 at 07:24:49PM CEST, j...@mojatatu.com wrote:

From: Jamal Hadi Salim 

Generic bitflags attribute content sent to the kernel by user.
With this netlink attr type the user can either set or unset a
flag in the kernel.

The value is a bitmap that defines the bit values being set
The selector is a bitmask that defines which value bit is to be
considered.

A check is made to ensure the rules that a kernel subsystem always
conforms to bitflags the kernel already knows about. i.e
if the user tries to set a bit flag that is not understood then
the _it will be rejected_.

In the most basic form, the user specifies the attribute policy as:
[ATTR_GOO] = { .type = NLA_BITFIELD32, .validation_data =  },

where myvalidflags is the bit mask of the flags the kernel understands.

If the user _does not_ provide myvalidflags then the attribute will
also be rejected.

Examples:
value = 0x0, and selector = 0x1
implies we are selecting bit 1 and we want to set its value to 0.

value = 0x2, and selector = 0x2
implies we are selecting bit 2 and we want to set its value to 1.

Suggested-by: Jiri Pirko 
Signed-off-by: Jamal Hadi Salim 
---
include/net/netlink.h| 16 
include/uapi/linux/netlink.h | 17 +
lib/nlattr.c | 30 ++
3 files changed, 63 insertions(+)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index ef8e6c3..82dd298 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -178,6 +178,7 @@ enum {
NLA_S16,
NLA_S32,
NLA_S64,
+   NLA_BITFIELD32,
__NLA_TYPE_MAX,
};

@@ -206,6 +207,7 @@ enum {
  *NLA_MSECSLeaving the length field zero will verify the
  * given type fits, using it verifies minimum length
  * just like "All other"
+ *NLA_BITFIELD32  A 32-bit bitmap/bitselector attribute
  *All otherMinimum length of attribute payload
  *
  * Example:
@@ -213,11 +215,13 @@ enum {
  * [ATTR_FOO] = { .type = NLA_U16 },
  * [ATTR_BAR] = { .type = NLA_STRING, .len = BARSIZ },
  * [ATTR_BAZ] = { .len = sizeof(struct mystruct) },
+ * [ATTR_GOO] = { .type = NLA_BITFIELD32, .validation_data =  
},


Checkpatch warns you about the line to long, please wrap it.

Btw, I did not see you reached a consensus with DavidA regarding this.
Did I miss it?



  * };
  */
struct nla_policy {
u16 type;
u16 len;
+   void*validation_data;
};

/**
@@ -1203,6 +1207,18 @@ static inline struct in6_addr nla_get_in6_addr(const 
struct nlattr *nla)
}

/**
+ * nla_get_bitfield32 - return payload of 32 bitfield attribute
+ * @nla: nla_bitfield32 attribute
+ */
+static inline struct nla_bitfield32 nla_get_bitfield32(const struct nlattr 
*nla)
+{
+   struct nla_bitfield32 tmp;
+
+   nla_memcpy(, nla, sizeof(tmp));
+   return tmp;
+}
+
+/**
  * nla_memdup - duplicate attribute memory (kmemdup)
  * @src: netlink attribute to duplicate from
  * @gfp: GFP mask
diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index f86127a..f4fc9c9 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -226,5 +226,22 @@ struct nlattr {
#define NLA_ALIGN(len)  (((len) + NLA_ALIGNTO - 1) & ~(NLA_ALIGNTO - 1))
#define NLA_HDRLEN  ((int) NLA_ALIGN(sizeof(struct nlattr)))

+/* Generic 32 bitflags attribute content sent to the kernel.
+ *
+ * The value is a bitmap that defines the values being set
+ * The selector is a bitmask that defines which value is legit
+ *
+ * Examples:
+ *  value = 0x0, and selector = 0x1
+ *  implies we are selecting bit 1 and we want to set its value to 0.
+ *
+ *  value = 0x2, and selector = 0x2
+ *  implies we are selecting bit 2 and we want to set its value to 1.
+ *
+ */
+struct nla_bitfield32 {
+   __u32 value;
+   __u32 selector;
+};

#endif /* _UAPI__LINUX_NETLINK_H */
diff --git a/lib/nlattr.c b/lib/nlattr.c
index fb52435..ee79b7a 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -27,6 +27,30 @@
[NLA_S64]   = sizeof(s64),
};

+static int validate_nla_bitfield32(const struct nlattr *nla,
+  u32 *valid_flags_allowed)
+{
+   const struct nla_bitfield32 *bf = nla_data(nla);
+   u32 *valid_flags_mask = valid_flags_allowed;


I pointed this out already. This weird.
You do *u32 = *u32, just with different name. Just use valid_flags_allowed

Re: [PATCH net-next v12 1/4] net netlink: Add new type NLA_BITFIELD32

[Jiri Pirko]

Sun, Jul 30, 2017 at 07:24:49PM CEST, j...@mojatatu.com wrote:
>From: Jamal Hadi Salim 
>
>Generic bitflags attribute content sent to the kernel by user.
>With this netlink attr type the user can either set or unset a
>flag in the kernel.
>
>The value is a bitmap that defines the bit values being set
>The selector is a bitmask that defines which value bit is to be
>considered.
>
>A check is made to ensure the rules that a kernel subsystem always
>conforms to bitflags the kernel already knows about. i.e
>if the user tries to set a bit flag that is not understood then
>the _it will be rejected_.
>
>In the most basic form, the user specifies the attribute policy as:
>[ATTR_GOO] = { .type = NLA_BITFIELD32, .validation_data =  },
>
>where myvalidflags is the bit mask of the flags the kernel understands.
>
>If the user _does not_ provide myvalidflags then the attribute will
>also be rejected.
>
>Examples:
>value = 0x0, and selector = 0x1
>implies we are selecting bit 1 and we want to set its value to 0.
>
>value = 0x2, and selector = 0x2
>implies we are selecting bit 2 and we want to set its value to 1.
>
>Suggested-by: Jiri Pirko 
>Signed-off-by: Jamal Hadi Salim 
>---
> include/net/netlink.h| 16 
> include/uapi/linux/netlink.h | 17 +
> lib/nlattr.c | 30 ++
> 3 files changed, 63 insertions(+)
>
>diff --git a/include/net/netlink.h b/include/net/netlink.h
>index ef8e6c3..82dd298 100644
>--- a/include/net/netlink.h
>+++ b/include/net/netlink.h
>@@ -178,6 +178,7 @@ enum {
>   NLA_S16,
>   NLA_S32,
>   NLA_S64,
>+  NLA_BITFIELD32,
>   __NLA_TYPE_MAX,
> };
> 
>@@ -206,6 +207,7 @@ enum {
>  *NLA_MSECSLeaving the length field zero will verify the
>  * given type fits, using it verifies minimum length
>  * just like "All other"
>+ *NLA_BITFIELD32  A 32-bit bitmap/bitselector attribute
>  *All otherMinimum length of attribute payload
>  *
>  * Example:
>@@ -213,11 +215,13 @@ enum {
>  *[ATTR_FOO] = { .type = NLA_U16 },
>  *[ATTR_BAR] = { .type = NLA_STRING, .len = BARSIZ },
>  *[ATTR_BAZ] = { .len = sizeof(struct mystruct) },
>+ *[ATTR_GOO] = { .type = NLA_BITFIELD32, .validation_data =  
>},

Checkpatch warns you about the line to long, please wrap it.

Btw, I did not see you reached a consensus with DavidA regarding this.
Did I miss it?


>  * };
>  */
> struct nla_policy {
>   u16 type;
>   u16 len;
>+  void*validation_data;
> };
> 
> /**
>@@ -1203,6 +1207,18 @@ static inline struct in6_addr nla_get_in6_addr(const 
>struct nlattr *nla)
> }
> 
> /**
>+ * nla_get_bitfield32 - return payload of 32 bitfield attribute
>+ * @nla: nla_bitfield32 attribute
>+ */
>+static inline struct nla_bitfield32 nla_get_bitfield32(const struct nlattr 
>*nla)
>+{
>+  struct nla_bitfield32 tmp;
>+
>+  nla_memcpy(, nla, sizeof(tmp));
>+  return tmp;
>+}
>+
>+/**
>  * nla_memdup - duplicate attribute memory (kmemdup)
>  * @src: netlink attribute to duplicate from
>  * @gfp: GFP mask
>diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
>index f86127a..f4fc9c9 100644
>--- a/include/uapi/linux/netlink.h
>+++ b/include/uapi/linux/netlink.h
>@@ -226,5 +226,22 @@ struct nlattr {
> #define NLA_ALIGN(len)(((len) + NLA_ALIGNTO - 1) & 
> ~(NLA_ALIGNTO - 1))
> #define NLA_HDRLEN((int) NLA_ALIGN(sizeof(struct nlattr)))
> 
>+/* Generic 32 bitflags attribute content sent to the kernel.
>+ *
>+ * The value is a bitmap that defines the values being set
>+ * The selector is a bitmask that defines which value is legit
>+ *
>+ * Examples:
>+ *  value = 0x0, and selector = 0x1
>+ *  implies we are selecting bit 1 and we want to set its value to 0.
>+ *
>+ *  value = 0x2, and selector = 0x2
>+ *  implies we are selecting bit 2 and we want to set its value to 1.
>+ *
>+ */
>+struct nla_bitfield32 {
>+  __u32 value;
>+  __u32 selector;
>+};
> 
> #endif /* _UAPI__LINUX_NETLINK_H */
>diff --git a/lib/nlattr.c b/lib/nlattr.c
>index fb52435..ee79b7a 100644
>--- a/lib/nlattr.c
>+++ b/lib/nlattr.c
>@@ -27,6 +27,30 @@
>   [NLA_S64]   = sizeof(s64),
> };
> 
>+static int validate_nla_bitfield32(const struct nlattr *nla,
>+ u32 *valid_flags_allowed)
>+{
>+  const struct nla_bitfield32 *bf = nla_data(nla);
>+  u32 *valid_flags_mask = valid_flags_allowed;

I pointed this out already. This weird.
You do *u32 = *u32, just with different name. Just use valid_flags_allowed
directly.


>+
>+  if (!valid_flags_allowed)
>+  return -EINVAL;
>+
>+  /*disallow invalid bit selector */

Fix all the comments in this function. Should be
/* something */
with spaces in front and at the end.


>+  if (bf->selector & ~*valid_flags_mask)
>+  

[PATCH net-next v12 1/4] net netlink: Add new type NLA_BITFIELD32

[Jamal Hadi Salim]

From: Jamal Hadi Salim 

Generic bitflags attribute content sent to the kernel by user.
With this netlink attr type the user can either set or unset a
flag in the kernel.

The value is a bitmap that defines the bit values being set
The selector is a bitmask that defines which value bit is to be
considered.

A check is made to ensure the rules that a kernel subsystem always
conforms to bitflags the kernel already knows about. i.e
if the user tries to set a bit flag that is not understood then
the _it will be rejected_.

In the most basic form, the user specifies the attribute policy as:
[ATTR_GOO] = { .type = NLA_BITFIELD32, .validation_data =  },

where myvalidflags is the bit mask of the flags the kernel understands.

If the user _does not_ provide myvalidflags then the attribute will
also be rejected.

Examples:
value = 0x0, and selector = 0x1
implies we are selecting bit 1 and we want to set its value to 0.

value = 0x2, and selector = 0x2
implies we are selecting bit 2 and we want to set its value to 1.

Suggested-by: Jiri Pirko 
Signed-off-by: Jamal Hadi Salim 
---
 include/net/netlink.h| 16 
 include/uapi/linux/netlink.h | 17 +
 lib/nlattr.c | 30 ++
 3 files changed, 63 insertions(+)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index ef8e6c3..82dd298 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -178,6 +178,7 @@ enum {
NLA_S16,
NLA_S32,
NLA_S64,
+   NLA_BITFIELD32,
__NLA_TYPE_MAX,
 };
 
@@ -206,6 +207,7 @@ enum {
  *NLA_MSECSLeaving the length field zero will verify the
  * given type fits, using it verifies minimum length
  * just like "All other"
+ *NLA_BITFIELD32  A 32-bit bitmap/bitselector attribute
  *All otherMinimum length of attribute payload
  *
  * Example:
@@ -213,11 +215,13 @@ enum {
  * [ATTR_FOO] = { .type = NLA_U16 },
  * [ATTR_BAR] = { .type = NLA_STRING, .len = BARSIZ },
  * [ATTR_BAZ] = { .len = sizeof(struct mystruct) },
+ * [ATTR_GOO] = { .type = NLA_BITFIELD32, .validation_data =  
},
  * };
  */
 struct nla_policy {
u16 type;
u16 len;
+   void*validation_data;
 };
 
 /**
@@ -1203,6 +1207,18 @@ static inline struct in6_addr nla_get_in6_addr(const 
struct nlattr *nla)
 }
 
 /**
+ * nla_get_bitfield32 - return payload of 32 bitfield attribute
+ * @nla: nla_bitfield32 attribute
+ */
+static inline struct nla_bitfield32 nla_get_bitfield32(const struct nlattr 
*nla)
+{
+   struct nla_bitfield32 tmp;
+
+   nla_memcpy(, nla, sizeof(tmp));
+   return tmp;
+}
+
+/**
  * nla_memdup - duplicate attribute memory (kmemdup)
  * @src: netlink attribute to duplicate from
  * @gfp: GFP mask
diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index f86127a..f4fc9c9 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -226,5 +226,22 @@ struct nlattr {
 #define NLA_ALIGN(len) (((len) + NLA_ALIGNTO - 1) & ~(NLA_ALIGNTO - 1))
 #define NLA_HDRLEN ((int) NLA_ALIGN(sizeof(struct nlattr)))
 
+/* Generic 32 bitflags attribute content sent to the kernel.
+ *
+ * The value is a bitmap that defines the values being set
+ * The selector is a bitmask that defines which value is legit
+ *
+ * Examples:
+ *  value = 0x0, and selector = 0x1
+ *  implies we are selecting bit 1 and we want to set its value to 0.
+ *
+ *  value = 0x2, and selector = 0x2
+ *  implies we are selecting bit 2 and we want to set its value to 1.
+ *
+ */
+struct nla_bitfield32 {
+   __u32 value;
+   __u32 selector;
+};
 
 #endif /* _UAPI__LINUX_NETLINK_H */
diff --git a/lib/nlattr.c b/lib/nlattr.c
index fb52435..ee79b7a 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -27,6 +27,30 @@
[NLA_S64]   = sizeof(s64),
 };
 
+static int validate_nla_bitfield32(const struct nlattr *nla,
+  u32 *valid_flags_allowed)
+{
+   const struct nla_bitfield32 *bf = nla_data(nla);
+   u32 *valid_flags_mask = valid_flags_allowed;
+
+   if (!valid_flags_allowed)
+   return -EINVAL;
+
+   /*disallow invalid bit selector */
+   if (bf->selector & ~*valid_flags_mask)
+   return -EINVAL;
+
+   /*disallow invalid bit values */
+   if (bf->value & ~*valid_flags_mask)
+   return -EINVAL;
+
+   /*disallow valid bit values that are not selected*/
+   if (bf->value & ~bf->selector)
+   return -EINVAL;
+
+   return 0;
+}
+
 static int validate_nla(const struct nlattr *nla, int maxtype,
const struct nla_policy *policy)
 {
@@ -46,6 +70,12 @@ static int validate_nla(const struct nlattr *nla, int 
maxtype,
return -ERANGE;
break;
 
+