| Message ID | 1497182026-11594-2-git-send-email-jhs@emojatatu.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Sun, Jun 11, 2017 at 01:53:43PM CEST, jhs@mojatatu.com wrote: >From: Jamal Hadi Salim <jhs@mojatatu.com> > >Generic bitflags attribute content sent to the kernel by user. >With this type the user can either set or unset a flag in the >kernel. > >The nla_flag_values is a bitmap that defines the values being set >The nla_flag_selector is a bitmask that defines which value is legit. > >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_FLAG_BITS, .validation_data = &myvalidflags }, > >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: >nla_flag_values = 0x0, and nla_flag_selector = 0x1 >implies we are selecting bit 1 and we want to set its value to 0. > >nla_flag_values = 0x2, and nla_flag_selector = 0x2 >implies we are selecting bit 2 and we want to set its value to 1. > >This patch also provides an extra feature: a validation callback >that could be speaciliazed for other types. s/speaciliazed/speciliazed/ >This feature is intended to be used by a kernel subsystem to check >for a combination of bits being present. Example "bit x is valid >only if bit y and z are present". > >So a kernel subsystem could specify validation rules of the following >nature: > >[ATTR_GOO] = { .type = MYTYPE, > .validation_data = &myvalidation_data, > .validate_content = mycontent_validator }, Indent is wrong. (Does not matter really in desc, but anyway) > >With validator callback looking like: > >int mycontent_validator(const struct nlattr *nla, void *valid_data) >{ > const struct myattribute *user_data = nla_data(nla); > struct myvalidation_struct *valid_data_constraint = valid_data; > > ... return appropriate error code etc ... >} > > >Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com> Suggested-by: Jiri Pirko <jiri@mellanox.com> >--- > include/net/netlink.h | 11 +++++++++++ > include/uapi/linux/rtnetlink.h | 17 +++++++++++++++++ > lib/nlattr.c | 25 +++++++++++++++++++++++++ > 3 files changed, 53 insertions(+) > >diff --git a/include/net/netlink.h b/include/net/netlink.h >index 0170917..8ab9784 100644 >--- a/include/net/netlink.h >+++ b/include/net/netlink.h >@@ -6,6 +6,11 @@ > #include <linux/jiffies.h> > #include <linux/in6.h> > >+struct nla_bit_flags { >+ u32 nla_flag_values; >+ u32 nla_flag_selector; >+}; I don't understand why you redefine the struct here. You already have it defined in the uapi: struct __nla_bit_flags Just move this (struct nla_bit_flags) to the uapi and remove __nla_bit_flags ? >+ > /* ======================================================================== > * Netlink Messages and Attributes Interface (As Seen On TV) > * ------------------------------------------------------------------------ >@@ -178,6 +183,7 @@ enum { > NLA_S16, > NLA_S32, > NLA_S64, >+ NLA_FLAG_BITS, > __NLA_TYPE_MAX, > }; > >@@ -206,6 +212,7 @@ enum { > * NLA_MSECS Leaving the length field zero will verify the > * given type fits, using it verifies minimum length > * just like "All other" >+ * NLA_FLAG_BITS A bitmap/bitselector attribute > * All other Minimum length of attribute payload > * > * Example: >@@ -213,11 +220,15 @@ enum { > * [ATTR_FOO] = { .type = NLA_U16 }, > * [ATTR_BAR] = { .type = NLA_STRING, .len = BARSIZ }, > * [ATTR_BAZ] = { .len = sizeof(struct mystruct) }, >+ * [ATTR_GOO] = { .type = NLA_FLAG_BITS, .validation_data = &myvalidflags }, > * }; > */ > struct nla_policy { > u16 type; > u16 len; >+ void *validation_data; >+ int (*validate_content)(const struct nlattr *nla, >+ const void *validation_data); > }; > > /** >diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h >index 564790e..8f07957 100644 >--- a/include/uapi/linux/rtnetlink.h >+++ b/include/uapi/linux/rtnetlink.h >@@ -179,6 +179,23 @@ struct rtattr { > #define RTA_DATA(rta) ((void*)(((char*)(rta)) + RTA_LENGTH(0))) > #define RTA_PAYLOAD(rta) ((int)((rta)->rta_len) - RTA_LENGTH(0)) > >+/* Generic bitflags attribute content sent to the kernel. >+ * >+ * The nla_flag_values is a bitmap that defines the values being set >+ * The nla_flag_selector is a bitmask that defines which value is legit >+ * >+ * Examples: >+ * nla_flag_values = 0x0, and nla_flag_selector = 0x1 >+ * implies we are selecting bit 1 and we want to set its value to 0. >+ * >+ * nla_flag_values = 0x2, and nla_flag_selector = 0x2 >+ * implies we are selecting bit 2 and we want to set its value to 1. >+ * >+ */ >+struct __nla_bit_flags { >+ __u32 nla_flag_values; >+ __u32 nla_flag_selector; >+}; > > > >diff --git a/lib/nlattr.c b/lib/nlattr.c >index a7e0b16..78fed43 100644 >--- a/lib/nlattr.c >+++ b/lib/nlattr.c >@@ -27,6 +27,21 @@ > [NLA_S64] = sizeof(s64), > }; > >+static int validate_nla_bit_flags(const struct nlattr *nla, void *valid_data) >+{ >+ const struct nla_bit_flags *nbf = nla_data(nla); >+ u32 *valid_flags_mask = valid_data; >+ >+ if (!valid_data) >+ return -EINVAL; >+ >+ Avoid one empty line here (you have 2) >+ if (nbf->nla_flag_values & ~*valid_flags_mask) >+ return -EINVAL; >+ >+ return 0; >+} >+ > static int validate_nla(const struct nlattr *nla, int maxtype, > const struct nla_policy *policy) > { >@@ -46,6 +61,13 @@ static int validate_nla(const struct nlattr *nla, int maxtype, > return -ERANGE; > break; > >+ case NLA_FLAG_BITS: >+ if (attrlen != 8) /* 2 x 32 bits */ sizeof(struct nla_bit_flags) instead of 8 please, you can skip the comment then. >+ return -ERANGE; >+ >+ return validate_nla_bit_flags(nla, pt->validation_data); >+ break; >+ > case NLA_NUL_STRING: > if (pt->len) > minlen = min_t(int, attrlen, pt->len + 1); >@@ -103,6 +125,9 @@ static int validate_nla(const struct nlattr *nla, int maxtype, > return -ERANGE; > } > >+ if (pt->validate_content) >+ return pt->validate_content(nla, pt->validation_data); This validation mechanism is completely independent from the added NLA_FLAG_BITS attr as it could be used with other attribute types. Please have it as a separate patch. >+ > return 0; > } > >-- >1.9.1 >
On 17-06-11 09:49 AM, Jiri Pirko wrote: > Sun, Jun 11, 2017 at 01:53:43PM CEST, jhs@mojatatu.com wrote: >> From: Jamal Hadi Salim <jhs@mojatatu.com> >> This patch also provides an extra feature: a validation callback >> that could be speaciliazed for other types. > > s/speaciliazed/speciliazed/ > Will fix. >> >> [ATTR_GOO] = { .type = MYTYPE, >> .validation_data = &myvalidation_data, >> .validate_content = mycontent_validator }, > > Indent is wrong. (Does not matter really in desc, but anyway) > I cant find out how it got indented that way; my source or email dont show it as such (but really doesnt matter). > Suggested-by: Jiri Pirko <jiri@mellanox.com> > Will add. > >> --- >> include/net/netlink.h | 11 +++++++++++ >> include/uapi/linux/rtnetlink.h | 17 +++++++++++++++++ >> lib/nlattr.c | 25 +++++++++++++++++++++++++ >> 3 files changed, 53 insertions(+) >> >> diff --git a/include/net/netlink.h b/include/net/netlink.h >> index 0170917..8ab9784 100644 >> --- a/include/net/netlink.h >> +++ b/include/net/netlink.h >> @@ -6,6 +6,11 @@ >> #include <linux/jiffies.h> >> #include <linux/in6.h> >> >> +struct nla_bit_flags { >> + u32 nla_flag_values; >> + u32 nla_flag_selector; >> +}; > > I don't understand why you redefine the struct here. You already have it > defined in the uapi: struct __nla_bit_flags > > Just move this (struct nla_bit_flags) to the uapi and remove > __nla_bit_flags ? > I am not sure that will compile since the type is defined in netlink.h Also, note: uapi uses _u32 and kernel uses u32 as types i.e it is pretty common approach; i will try to move it to uapi and keep that uapi format. If it doesnt compile without acrobatics I will keep it as is. > > >> + >> diff --git a/lib/nlattr.c b/lib/nlattr.c >> index a7e0b16..78fed43 100644 >> --- a/lib/nlattr.c >> +++ b/lib/nlattr.c >> @@ -27,6 +27,21 @@ >> [NLA_S64] = sizeof(s64), >> }; >> >> +static int validate_nla_bit_flags(const struct nlattr *nla, void *valid_data) >> +{ >> + const struct nla_bit_flags *nbf = nla_data(nla); >> + u32 *valid_flags_mask = valid_data; >> + >> + if (!valid_data) >> + return -EINVAL; >> + >> + > > Avoid one empty line here (you have 2) > Will fix. > >> + if (nbf->nla_flag_values & ~*valid_flags_mask) >> + return -EINVAL; >> + >> + return 0; >> +} >> + >> static int validate_nla(const struct nlattr *nla, int maxtype, >> const struct nla_policy *policy) >> { >> @@ -46,6 +61,13 @@ static int validate_nla(const struct nlattr *nla, int maxtype, >> return -ERANGE; >> break; >> >> + case NLA_FLAG_BITS: >> + if (attrlen != 8) /* 2 x 32 bits */ > > sizeof(struct nla_bit_flags) instead of 8 please, you can skip the > comment then. > Good point. > >> + return -ERANGE; >> + >> + return validate_nla_bit_flags(nla, pt->validation_data); >> + break; >> + >> case NLA_NUL_STRING: >> if (pt->len) >> minlen = min_t(int, attrlen, pt->len + 1); >> @@ -103,6 +125,9 @@ static int validate_nla(const struct nlattr *nla, int maxtype, >> return -ERANGE; >> } >> >> + if (pt->validate_content) >> + return pt->validate_content(nla, pt->validation_data); > > This validation mechanism is completely independent from the added NLA_FLAG_BITS > attr as it could be used with other attribute types. Please have it as a > separate patch. > Ok - so one more patch then ;-> cheers, jamal
On 17-06-11 01:38 PM, Jamal Hadi Salim wrote: > On 17-06-11 09:49 AM, Jiri Pirko wrote: >> Sun, Jun 11, 2017 at 01:53:43PM CEST, jhs@mojatatu.com wrote: >>> From: Jamal Hadi Salim <jhs@mojatatu.com> > > >>> This patch also provides an extra feature: a validation callback >>> that could be speaciliazed for other types. >> >> s/speaciliazed/speciliazed/ >> > > Will fix. > > >>> >>> [ATTR_GOO] = { .type = MYTYPE, >>> .validation_data = &myvalidation_data, >>> .validate_content = mycontent_validator }, >> >> Indent is wrong. (Does not matter really in desc, but anyway) >> > > I cant find out how it got indented that way; my source > or email dont show it as such (but really doesnt matter). > > >> Suggested-by: Jiri Pirko <jiri@mellanox.com> >> > > Will add. > >> >>> --- >>> include/net/netlink.h | 11 +++++++++++ >>> include/uapi/linux/rtnetlink.h | 17 +++++++++++++++++ >>> lib/nlattr.c | 25 +++++++++++++++++++++++++ >>> 3 files changed, 53 insertions(+) >>> >>> diff --git a/include/net/netlink.h b/include/net/netlink.h >>> index 0170917..8ab9784 100644 >>> --- a/include/net/netlink.h >>> +++ b/include/net/netlink.h >>> @@ -6,6 +6,11 @@ >>> #include <linux/jiffies.h> >>> #include <linux/in6.h> >>> >>> +struct nla_bit_flags { >>> + u32 nla_flag_values; >>> + u32 nla_flag_selector; >>> +}; >> >> I don't understand why you redefine the struct here. You already have it >> defined in the uapi: struct __nla_bit_flags >> >> Just move this (struct nla_bit_flags) to the uapi and remove >> __nla_bit_flags ? >> > > I am not sure that will compile since the type is defined in netlink.h > Also, note: uapi uses _u32 and kernel uses u32 as types i.e it is pretty > common approach; i will try to move it to uapi and keep that uapi > format. If it doesnt compile without acrobatics I will keep it as is. > It doesnt compile - I could move it to linux/netlink.h but it seems so out of place. so i will keep things as is for now unless you can think of something else. cheers, jamal
Sun, Jun 11, 2017 at 08:37:25PM CEST, jhs@mojatatu.com wrote: >On 17-06-11 01:38 PM, Jamal Hadi Salim wrote: >> On 17-06-11 09:49 AM, Jiri Pirko wrote: >> > Sun, Jun 11, 2017 at 01:53:43PM CEST, jhs@mojatatu.com wrote: >> > > From: Jamal Hadi Salim <jhs@mojatatu.com> >> >> >> > > This patch also provides an extra feature: a validation callback >> > > that could be speaciliazed for other types. >> > >> > s/speaciliazed/speciliazed/ >> > >> >> Will fix. >> >> >> > > >> > > [ATTR_GOO] = { .type = MYTYPE, >> > > .validation_data = &myvalidation_data, >> > > .validate_content = mycontent_validator }, >> > >> > Indent is wrong. (Does not matter really in desc, but anyway) >> > >> >> I cant find out how it got indented that way; my source >> or email dont show it as such (but really doesnt matter). >> >> >> > Suggested-by: Jiri Pirko <jiri@mellanox.com> >> > >> >> Will add. >> >> > >> > > --- >> > > include/net/netlink.h | 11 +++++++++++ >> > > include/uapi/linux/rtnetlink.h | 17 +++++++++++++++++ >> > > lib/nlattr.c | 25 +++++++++++++++++++++++++ >> > > 3 files changed, 53 insertions(+) >> > > >> > > diff --git a/include/net/netlink.h b/include/net/netlink.h >> > > index 0170917..8ab9784 100644 >> > > --- a/include/net/netlink.h >> > > +++ b/include/net/netlink.h >> > > @@ -6,6 +6,11 @@ >> > > #include <linux/jiffies.h> >> > > #include <linux/in6.h> >> > > >> > > +struct nla_bit_flags { >> > > + u32 nla_flag_values; >> > > + u32 nla_flag_selector; >> > > +}; >> > >> > I don't understand why you redefine the struct here. You already have it >> > defined in the uapi: struct __nla_bit_flags >> > >> > Just move this (struct nla_bit_flags) to the uapi and remove >> > __nla_bit_flags ? >> > >> >> I am not sure that will compile since the type is defined in netlink.h >> Also, note: uapi uses _u32 and kernel uses u32 as types i.e it is pretty >> common approach; i will try to move it to uapi and keep that uapi >> format. If it doesnt compile without acrobatics I will keep it as is. >> > >It doesnt compile - I could move it to linux/netlink.h but it seems >so out of place. >so i will keep things as is for now unless you can think of something >else. First of all, makes no sense to put this struct "struct __nla_bit_flags" into rtnetlink.h uapi file. This is generic netlink stuff, not specifict to rtnetlink. I believe that this struct should go into: include/uapi/linux/netlink.h struct nla_flag_bits { __u32 nla_flag_bits_values; __u32 nla_flag_bits_selector; }; Then you can use it from userspace and everywhere in kernel. Btw, I find it very odd that enum containling NLA_* like NLA_U32 and others is not part of uapi file and is rather defined in include/net/netlink.h. Any idea why?
On 17-06-12 06:34 AM, Jiri Pirko wrote: > Sun, Jun 11, 2017 at 08:37:25PM CEST, jhs@mojatatu.com wrote: >> On 17-06-11 01:38 PM, Jamal Hadi Salim wrote: >>> On 17-06-11 09:49 AM, Jiri Pirko wrote: >>>> Sun, Jun 11, 2017 at 01:53:43PM CEST, jhs@mojatatu.com wrote: >>>>> From: Jamal Hadi Salim <jhs@mojatatu.com> >>> >>> >>>>> This patch also provides an extra feature: a validation callback >>>>> that could be speaciliazed for other types. >>>> >>>> s/speaciliazed/speciliazed/ >>>> >>> >>> Will fix. >>> >>> >>>>> >>>>> [ATTR_GOO] = { .type = MYTYPE, >>>>> .validation_data = &myvalidation_data, >>>>> .validate_content = mycontent_validator }, >>>> >>>> Indent is wrong. (Does not matter really in desc, but anyway) >>>> >>> >>> I cant find out how it got indented that way; my source >>> or email dont show it as such (but really doesnt matter). >>> >>> >>>> Suggested-by: Jiri Pirko <jiri@mellanox.com> >>>> >>> >>> Will add. >>> >>>> >>>>> --- >>>>> include/net/netlink.h | 11 +++++++++++ >>>>> include/uapi/linux/rtnetlink.h | 17 +++++++++++++++++ >>>>> lib/nlattr.c | 25 +++++++++++++++++++++++++ >>>>> 3 files changed, 53 insertions(+) >>>>> >>>>> diff --git a/include/net/netlink.h b/include/net/netlink.h >>>>> index 0170917..8ab9784 100644 >>>>> --- a/include/net/netlink.h >>>>> +++ b/include/net/netlink.h >>>>> @@ -6,6 +6,11 @@ >>>>> #include <linux/jiffies.h> >>>>> #include <linux/in6.h> >>>>> >>>>> +struct nla_bit_flags { >>>>> + u32 nla_flag_values; >>>>> + u32 nla_flag_selector; >>>>> +}; >>>> >>>> I don't understand why you redefine the struct here. You already have it >>>> defined in the uapi: struct __nla_bit_flags >>>> >>>> Just move this (struct nla_bit_flags) to the uapi and remove >>>> __nla_bit_flags ? >>>> >>> >>> I am not sure that will compile since the type is defined in netlink.h >>> Also, note: uapi uses _u32 and kernel uses u32 as types i.e it is pretty >>> common approach; i will try to move it to uapi and keep that uapi >>> format. If it doesnt compile without acrobatics I will keep it as is. >>> >> >> It doesnt compile - I could move it to linux/netlink.h but it seems >> so out of place. >> so i will keep things as is for now unless you can think of something >> else. > > First of all, makes no sense to put this struct "struct __nla_bit_flags" > into rtnetlink.h uapi file. This is generic netlink stuff, not specifict > to rtnetlink. > > I believe that this struct should go into: > include/uapi/linux/netlink.h > > struct nla_flag_bits { > __u32 nla_flag_bits_values; > __u32 nla_flag_bits_selector; > }; > > Then you can use it from userspace and everywhere in kernel. > That file seems to be very out of place for this stuff. > Btw, I find it very odd that enum containling NLA_* like NLA_U32 and > others is not part of uapi file and is rather defined in > include/net/netlink.h. Any idea why? > NLA_XXX are kernel side types. They are part of net/netlink.h which is not uapi accessible. David Ahern has submitted a patch to move all those defines to iproute2. Will make sense to move these to a uapi/linux/netlink-types.h but that is waay beyond the scope of this patch set. cheers, jamal
Mon, Jun 12, 2017 at 01:10:56PM CEST, jhs@mojatatu.com wrote: >On 17-06-12 06:34 AM, Jiri Pirko wrote: >> Sun, Jun 11, 2017 at 08:37:25PM CEST, jhs@mojatatu.com wrote: >> > On 17-06-11 01:38 PM, Jamal Hadi Salim wrote: >> > > On 17-06-11 09:49 AM, Jiri Pirko wrote: >> > > > Sun, Jun 11, 2017 at 01:53:43PM CEST, jhs@mojatatu.com wrote: >> > > > > From: Jamal Hadi Salim <jhs@mojatatu.com> >> > > >> > > >> > > > > This patch also provides an extra feature: a validation callback >> > > > > that could be speaciliazed for other types. >> > > > >> > > > s/speaciliazed/speciliazed/ >> > > > >> > > >> > > Will fix. >> > > >> > > >> > > > > >> > > > > [ATTR_GOO] = { .type = MYTYPE, >> > > > > .validation_data = &myvalidation_data, >> > > > > .validate_content = mycontent_validator }, >> > > > >> > > > Indent is wrong. (Does not matter really in desc, but anyway) >> > > > >> > > >> > > I cant find out how it got indented that way; my source >> > > or email dont show it as such (but really doesnt matter). >> > > >> > > >> > > > Suggested-by: Jiri Pirko <jiri@mellanox.com> >> > > > >> > > >> > > Will add. >> > > >> > > > >> > > > > --- >> > > > > include/net/netlink.h | 11 +++++++++++ >> > > > > include/uapi/linux/rtnetlink.h | 17 +++++++++++++++++ >> > > > > lib/nlattr.c | 25 +++++++++++++++++++++++++ >> > > > > 3 files changed, 53 insertions(+) >> > > > > >> > > > > diff --git a/include/net/netlink.h b/include/net/netlink.h >> > > > > index 0170917..8ab9784 100644 >> > > > > --- a/include/net/netlink.h >> > > > > +++ b/include/net/netlink.h >> > > > > @@ -6,6 +6,11 @@ >> > > > > #include <linux/jiffies.h> >> > > > > #include <linux/in6.h> >> > > > > >> > > > > +struct nla_bit_flags { >> > > > > + u32 nla_flag_values; >> > > > > + u32 nla_flag_selector; >> > > > > +}; >> > > > >> > > > I don't understand why you redefine the struct here. You already have it >> > > > defined in the uapi: struct __nla_bit_flags >> > > > >> > > > Just move this (struct nla_bit_flags) to the uapi and remove >> > > > __nla_bit_flags ? >> > > > >> > > >> > > I am not sure that will compile since the type is defined in netlink.h >> > > Also, note: uapi uses _u32 and kernel uses u32 as types i.e it is pretty >> > > common approach; i will try to move it to uapi and keep that uapi >> > > format. If it doesnt compile without acrobatics I will keep it as is. >> > > >> > >> > It doesnt compile - I could move it to linux/netlink.h but it seems >> > so out of place. >> > so i will keep things as is for now unless you can think of something >> > else. >> >> First of all, makes no sense to put this struct "struct __nla_bit_flags" >> into rtnetlink.h uapi file. This is generic netlink stuff, not specifict >> to rtnetlink. >> >> I believe that this struct should go into: >> include/uapi/linux/netlink.h >> >> struct nla_flag_bits { >> __u32 nla_flag_bits_values; >> __u32 nla_flag_bits_selector; >> }; >> >> Then you can use it from userspace and everywhere in kernel. >> >That file seems to be very out of place for this stuff. Howcome? It is a common netlink api. > >> Btw, I find it very odd that enum containling NLA_* like NLA_U32 and >> others is not part of uapi file and is rather defined in >> include/net/netlink.h. Any idea why? >> > >NLA_XXX are kernel side types. They are part of net/netlink.h which is >not uapi accessible. >David Ahern has submitted a patch to move all those defines to iproute2. >Will make sense to move these to a uapi/linux/netlink-types.h but that >is waay beyond the scope of this patch set. Well, I don't think so :) The thing is, struct nla_flag_bits is tightly coupled with NLA_FLAG_BITS enum value. They should be in the same uapi file. That makes sense to me.
On 17-06-12 07:43 AM, Jiri Pirko wrote: > Mon, Jun 12, 2017 at 01:10:56PM CEST, jhs@mojatatu.com wrote: >> On 17-06-12 06:34 AM, Jiri Pirko wrote: >>> Sun, Jun 11, 2017 at 08:37:25PM CEST, jhs@mojatatu.com wrote: >>>> On 17-06-11 01:38 PM, Jamal Hadi Salim wrote: >>>>> On 17-06-11 09:49 AM, Jiri Pirko wrote: >>>>>> Sun, Jun 11, 2017 at 01:53:43PM CEST, jhs@mojatatu.com wrote: >>>>>>> From: Jamal Hadi Salim <jhs@mojatatu.com> >>>>> >>>>> >>>>>>> This patch also provides an extra feature: a validation callback >>>>>>> that could be speaciliazed for other types. >>>>>> >>>>>> s/speaciliazed/speciliazed/ >>>>>> >>>>> >>>>> Will fix. >>>>> >>>>> >>>>>>> >>>>>>> [ATTR_GOO] = { .type = MYTYPE, >>>>>>> .validation_data = &myvalidation_data, >>>>>>> .validate_content = mycontent_validator }, >>>>>> >>>>>> Indent is wrong. (Does not matter really in desc, but anyway) >>>>>> >>>>> >>>>> I cant find out how it got indented that way; my source >>>>> or email dont show it as such (but really doesnt matter). >>>>> >>>>> >>>>>> Suggested-by: Jiri Pirko <jiri@mellanox.com> >>>>>> >>>>> >>>>> Will add. >>>>> >>>>>> >>> I believe that this struct should go into: >>> include/uapi/linux/netlink.h >>> >>> struct nla_flag_bits { >>> __u32 nla_flag_bits_values; >>> __u32 nla_flag_bits_selector; >>> }; >>> >>> Then you can use it from userspace and everywhere in kernel. >>> >> That file seems to be very out of place for this stuff. > > Howcome? It is a common netlink api. > Take a look at that file's content. It talks about what goes in the netlink header. Adding types in it seems out of place. >> NLA_XXX are kernel side types. They are part of net/netlink.h which is >> not uapi accessible. >> David Ahern has submitted a patch to move all those defines to iproute2. >> Will make sense to move these to a uapi/linux/netlink-types.h but that >> is waay beyond the scope of this patch set. > > Well, I don't think so :) > > The thing is, struct nla_flag_bits is tightly coupled with NLA_FLAG_BITS > enum value. They should be in the same uapi file. That makes sense to me. > Sure - they should be in the same file. But is it uapi/linux/netlink.h? cheers, jamal
Mon, Jun 12, 2017 at 03:51:19PM CEST, jhs@mojatatu.com wrote: >On 17-06-12 07:43 AM, Jiri Pirko wrote: >> Mon, Jun 12, 2017 at 01:10:56PM CEST, jhs@mojatatu.com wrote: >> > On 17-06-12 06:34 AM, Jiri Pirko wrote: >> > > Sun, Jun 11, 2017 at 08:37:25PM CEST, jhs@mojatatu.com wrote: >> > > > On 17-06-11 01:38 PM, Jamal Hadi Salim wrote: >> > > > > On 17-06-11 09:49 AM, Jiri Pirko wrote: >> > > > > > Sun, Jun 11, 2017 at 01:53:43PM CEST, jhs@mojatatu.com wrote: >> > > > > > > From: Jamal Hadi Salim <jhs@mojatatu.com> >> > > > > >> > > > > >> > > > > > > This patch also provides an extra feature: a validation callback >> > > > > > > that could be speaciliazed for other types. >> > > > > > >> > > > > > s/speaciliazed/speciliazed/ >> > > > > > >> > > > > >> > > > > Will fix. >> > > > > >> > > > > >> > > > > > > >> > > > > > > [ATTR_GOO] = { .type = MYTYPE, >> > > > > > > .validation_data = &myvalidation_data, >> > > > > > > .validate_content = mycontent_validator }, >> > > > > > >> > > > > > Indent is wrong. (Does not matter really in desc, but anyway) >> > > > > > >> > > > > >> > > > > I cant find out how it got indented that way; my source >> > > > > or email dont show it as such (but really doesnt matter). >> > > > > >> > > > > >> > > > > > Suggested-by: Jiri Pirko <jiri@mellanox.com> >> > > > > > >> > > > > >> > > > > Will add. >> > > > > >> > > > > > > >> > > I believe that this struct should go into: >> > > include/uapi/linux/netlink.h >> > > >> > > struct nla_flag_bits { >> > > __u32 nla_flag_bits_values; >> > > __u32 nla_flag_bits_selector; >> > > }; >> > > >> > > Then you can use it from userspace and everywhere in kernel. >> > > >> > That file seems to be very out of place for this stuff. >> >> Howcome? It is a common netlink api. >> > >Take a look at that file's content. It talks about what goes in >the netlink header. Adding types in it seems out of place. Bit less than rtnetlink.h. But I agree it is not optimal. Re optimal, please see below. > > >> > NLA_XXX are kernel side types. They are part of net/netlink.h which is >> > not uapi accessible. >> > David Ahern has submitted a patch to move all those defines to iproute2. >> > Will make sense to move these to a uapi/linux/netlink-types.h but that >> > is waay beyond the scope of this patch set. >> >> Well, I don't think so :) >> >> The thing is, struct nla_flag_bits is tightly coupled with NLA_FLAG_BITS >> enum value. They should be in the same uapi file. That makes sense to me. >> > >Sure - they should be in the same file. But is it uapi/linux/netlink.h? Might be the netlink-types.h you mentioned above. ccing DavidA.
On 6/12/17 8:14 AM, Jiri Pirko wrote: >>> The thing is, struct nla_flag_bits is tightly coupled with NLA_FLAG_BITS >>> enum value. They should be in the same uapi file. That makes sense to me. >>> >> >> Sure - they should be in the same file. But is it uapi/linux/netlink.h? > > Might be the netlink-types.h you mentioned above. > > ccing DavidA. > Just saw this patch set this morning. Few comments: 1. I think nla_bitfield or nla_bitmap is a better name than nla_flag_bits 2. The length should be open ended with the size of the array determined by nla_len / sizeof(struct nla_bitfield). That allows this to be extended to an arbitrary large bitfield as needed. 3. IMO since these are nla prefixes and new NLA type they should be in uapi/linux/netlink.h
Mon, Jun 12, 2017 at 05:00:41PM CEST, dsahern@gmail.com wrote: >On 6/12/17 8:14 AM, Jiri Pirko wrote: >>>> The thing is, struct nla_flag_bits is tightly coupled with NLA_FLAG_BITS >>>> enum value. They should be in the same uapi file. That makes sense to me. >>>> >>> >>> Sure - they should be in the same file. But is it uapi/linux/netlink.h? >> >> Might be the netlink-types.h you mentioned above. >> >> ccing DavidA. >> > >Just saw this patch set this morning. Few comments: > >1. I think nla_bitfield or nla_bitmap is a better name than nla_flag_bits ack > >2. The length should be open ended with the size of the array determined >by nla_len / sizeof(struct nla_bitfield). That allows this to be >extended to an arbitrary large bitfield as needed. Yeah, I was thinking about that as well. Seems handy to have this generic len. > >3. IMO since these are nla prefixes and new NLA type they should be in >uapi/linux/netlink.h Including NLA_* type enum? I think it is reasonable.
On 6/12/17 1:22 PM, Jiri Pirko wrote: > >> 3. IMO since these are nla prefixes and new NLA type they should be in >> uapi/linux/netlink.h > Including NLA_* type enum? I think it is reasonable. well, maybe not the NLA_BITFIELD. That enum is for policy validation kernel side so not really part of the API.
Mon, Jun 12, 2017 at 09:58:33PM CEST, dsahern@gmail.com wrote: >On 6/12/17 1:22 PM, Jiri Pirko wrote: >> >>> 3. IMO since these are nla prefixes and new NLA type they should be in >>> uapi/linux/netlink.h >> Including NLA_* type enum? I think it is reasonable. > >well, maybe not the NLA_BITFIELD. That enum is for policy validation >kernel side so not really part of the API. Yeah, now I see it. Agreed.
On 17-06-13 01:32 AM, Jiri Pirko wrote: > Mon, Jun 12, 2017 at 09:58:33PM CEST, dsahern@gmail.com wrote: >> On 6/12/17 1:22 PM, Jiri Pirko wrote: >>> >>>> 3. IMO since these are nla prefixes and new NLA type they should be in >>>> uapi/linux/netlink.h >>> Including NLA_* type enum? I think it is reasonable. >> >> well, maybe not the NLA_BITFIELD. That enum is for policy validation >> kernel side so not really part of the API. > > Yeah, now I see it. Agreed. > Jiri, you agreed to a name change? ;-> I want to have some of what David A. ate yesterday. I agree it is a good idea to have arbitrary size bitmask so we dont run out of bit space but we need to restrict the max length possible. I dont agree to using the netlink.h as the best location for this but lets move on. Do you or David A. want to take a crack at this? I am a little tied up. cheers, jamal
Tue, Jun 13, 2017 at 12:53:00PM CEST, jhs@mojatatu.com wrote: >On 17-06-13 01:32 AM, Jiri Pirko wrote: >> Mon, Jun 12, 2017 at 09:58:33PM CEST, dsahern@gmail.com wrote: >> > On 6/12/17 1:22 PM, Jiri Pirko wrote: >> > > >> > > > 3. IMO since these are nla prefixes and new NLA type they should be in >> > > > uapi/linux/netlink.h >> > > Including NLA_* type enum? I think it is reasonable. >> > >> > well, maybe not the NLA_BITFIELD. That enum is for policy validation >> > kernel side so not really part of the API. >> >> Yeah, now I see it. Agreed. >> > >Jiri, you agreed to a name change? ;-> I want to have some of what >David A. ate yesterday. I ok with either of the tree (1 Jamal's, 2 DaveA's) variants. > >I agree it is a good idea to have arbitrary size bitmask so we dont >run out of bit space but we need to restrict the max length possible. >I dont agree to using the netlink.h as the best location for this >but lets move on. Suggest a better place, I don't see one. >Do you or David A. want to take a crack at this? I am a little tied >up. No time right now. Maybe in July I could allocate some cycles for this.
On 6/13/17 5:25 AM, Jiri Pirko wrote: > >> Do you or David A. want to take a crack at this? I am a little tied >> up. > No time right now. Maybe in July I could allocate some cycles for this. I'll see what I can do this weekend.
diff --git a/include/net/netlink.h b/include/net/netlink.h index 0170917..8ab9784 100644 --- a/include/net/netlink.h +++ b/include/net/netlink.h @@ -6,6 +6,11 @@ #include <linux/jiffies.h> #include <linux/in6.h> +struct nla_bit_flags { + u32 nla_flag_values; + u32 nla_flag_selector; +}; + /* ======================================================================== * Netlink Messages and Attributes Interface (As Seen On TV) * ------------------------------------------------------------------------ @@ -178,6 +183,7 @@ enum { NLA_S16, NLA_S32, NLA_S64, + NLA_FLAG_BITS, __NLA_TYPE_MAX, }; @@ -206,6 +212,7 @@ enum { * NLA_MSECS Leaving the length field zero will verify the * given type fits, using it verifies minimum length * just like "All other" + * NLA_FLAG_BITS A bitmap/bitselector attribute * All other Minimum length of attribute payload * * Example: @@ -213,11 +220,15 @@ enum { * [ATTR_FOO] = { .type = NLA_U16 }, * [ATTR_BAR] = { .type = NLA_STRING, .len = BARSIZ }, * [ATTR_BAZ] = { .len = sizeof(struct mystruct) }, + * [ATTR_GOO] = { .type = NLA_FLAG_BITS, .validation_data = &myvalidflags }, * }; */ struct nla_policy { u16 type; u16 len; + void *validation_data; + int (*validate_content)(const struct nlattr *nla, + const void *validation_data); }; /** diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h index 564790e..8f07957 100644 --- a/include/uapi/linux/rtnetlink.h +++ b/include/uapi/linux/rtnetlink.h @@ -179,6 +179,23 @@ struct rtattr { #define RTA_DATA(rta) ((void*)(((char*)(rta)) + RTA_LENGTH(0))) #define RTA_PAYLOAD(rta) ((int)((rta)->rta_len) - RTA_LENGTH(0)) +/* Generic bitflags attribute content sent to the kernel. + * + * The nla_flag_values is a bitmap that defines the values being set + * The nla_flag_selector is a bitmask that defines which value is legit + * + * Examples: + * nla_flag_values = 0x0, and nla_flag_selector = 0x1 + * implies we are selecting bit 1 and we want to set its value to 0. + * + * nla_flag_values = 0x2, and nla_flag_selector = 0x2 + * implies we are selecting bit 2 and we want to set its value to 1. + * + */ +struct __nla_bit_flags { + __u32 nla_flag_values; + __u32 nla_flag_selector; +}; diff --git a/lib/nlattr.c b/lib/nlattr.c index a7e0b16..78fed43 100644 --- a/lib/nlattr.c +++ b/lib/nlattr.c @@ -27,6 +27,21 @@ [NLA_S64] = sizeof(s64), }; +static int validate_nla_bit_flags(const struct nlattr *nla, void *valid_data) +{ + const struct nla_bit_flags *nbf = nla_data(nla); + u32 *valid_flags_mask = valid_data; + + if (!valid_data) + return -EINVAL; + + + if (nbf->nla_flag_values & ~*valid_flags_mask) + return -EINVAL; + + return 0; +} + static int validate_nla(const struct nlattr *nla, int maxtype, const struct nla_policy *policy) { @@ -46,6 +61,13 @@ static int validate_nla(const struct nlattr *nla, int maxtype, return -ERANGE; break; + case NLA_FLAG_BITS: + if (attrlen != 8) /* 2 x 32 bits */ + return -ERANGE; + + return validate_nla_bit_flags(nla, pt->validation_data); + break; + case NLA_NUL_STRING: if (pt->len) minlen = min_t(int, attrlen, pt->len + 1); @@ -103,6 +125,9 @@ static int validate_nla(const struct nlattr *nla, int maxtype, return -ERANGE; } + if (pt->validate_content) + return pt->validate_content(nla, pt->validation_data); + return 0; }