| Message ID | cover.1610978306.git.petrm@nvidia.org |
|---|---|
| Headers | show |
| Series | nexthop: More fine-grained policies for netlink message validation | expand |
On 1/18/21 7:05 AM, Petr Machata wrote: > This function uses the global nexthop policy only to then bounce all > arguments except for NHA_ID. Instead, just create a new policy that > only includes the one allowed attribute. > > Signed-off-by: Petr Machata <petrm@nvidia.com> > Reviewed-by: Ido Schimmel <idosch@nvidia.com> > --- > net/ipv4/nexthop.c | 21 ++++++--------------- > 1 file changed, 6 insertions(+), 15 deletions(-) > Reviewed-by: David Ahern <dsahern@kernel.org>
On Mon, 18 Jan 2021 15:05:23 +0100 Petr Machata wrote: > This function uses the global nexthop policy only to then bounce all > arguments except for NHA_ID. Instead, just create a new policy that > only includes the one allowed attribute. > > Signed-off-by: Petr Machata <petrm@nvidia.com> > Reviewed-by: Ido Schimmel <idosch@nvidia.com> > --- > net/ipv4/nexthop.c | 21 ++++++--------------- > 1 file changed, 6 insertions(+), 15 deletions(-) > > diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c > index e53e43aef785..d5d88f7c5c11 100644 > --- a/net/ipv4/nexthop.c > +++ b/net/ipv4/nexthop.c > @@ -36,6 +36,10 @@ static const struct nla_policy rtm_nh_policy[NHA_MAX + 1] = { > [NHA_FDB] = { .type = NLA_FLAG }, > }; > > +static const struct nla_policy rtm_nh_policy_get[NHA_MAX + 1] = { This is an unnecessary waste of memory if you ask me. NHA_ID is 1, so we're creating an array of 10 extra NULL elements. Can you leave the size to the compiler and use ARRAY_SIZE() below? > + [NHA_ID] = { .type = NLA_U32 }, > +}; > + > static bool nexthop_notifiers_is_empty(struct net *net) > { > return !net->nexthop.notifier_chain.head; > @@ -1843,27 +1847,14 @@ static int nh_valid_get_del_req(struct nlmsghdr *nlh, u32 *id, > { > struct nhmsg *nhm = nlmsg_data(nlh); > struct nlattr *tb[NHA_MAX + 1]; > - int err, i; > + int err; > > - err = nlmsg_parse(nlh, sizeof(*nhm), tb, NHA_MAX, rtm_nh_policy, > + err = nlmsg_parse(nlh, sizeof(*nhm), tb, NHA_MAX, rtm_nh_policy_get, > extack); > if (err < 0) > return err; > > err = -EINVAL; > - for (i = 0; i < __NHA_MAX; ++i) { > - if (!tb[i]) > - continue; > - > - switch (i) { > - case NHA_ID: > - break; > - default: > - NL_SET_ERR_MSG_ATTR(extack, tb[i], > - "Unexpected attribute in request"); > - goto out; > - } > - } > if (nhm->nh_protocol || nhm->resvd || nhm->nh_scope || nhm->nh_flags) { > NL_SET_ERR_MSG(extack, "Invalid values in header"); > goto out;
On Mon, 18 Jan 2021 15:05:24 +0100 Petr Machata wrote: > + if (tb[NHA_GROUPS]) > + *group_filter = true; > + if (tb[NHA_FDB]) > + *fdb_filter = true; nla_get_flag()
On 1/19/21 1:55 PM, Jakub Kicinski wrote: >> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c >> index e53e43aef785..d5d88f7c5c11 100644 >> --- a/net/ipv4/nexthop.c >> +++ b/net/ipv4/nexthop.c >> @@ -36,6 +36,10 @@ static const struct nla_policy rtm_nh_policy[NHA_MAX + 1] = { >> [NHA_FDB] = { .type = NLA_FLAG }, >> }; >> >> +static const struct nla_policy rtm_nh_policy_get[NHA_MAX + 1] = { > > This is an unnecessary waste of memory if you ask me. > > NHA_ID is 1, so we're creating an array of 10 extra NULL elements. > > Can you leave the size to the compiler and use ARRAY_SIZE() below? interesting suggestion in general for netlink attributes. > >> + [NHA_ID] = { .type = NLA_U32 }, >> +}; >> + >> static bool nexthop_notifiers_is_empty(struct net *net) >> { >> return !net->nexthop.notifier_chain.head; >> @@ -1843,27 +1847,14 @@ static int nh_valid_get_del_req(struct nlmsghdr *nlh, u32 *id, >> { >> struct nhmsg *nhm = nlmsg_data(nlh); >> struct nlattr *tb[NHA_MAX + 1]; This tb array too could be sized to just the highest indexed expected - NHA_ID in this case. >> - int err, i; >> + int err; >> >> - err = nlmsg_parse(nlh, sizeof(*nhm), tb, NHA_MAX, rtm_nh_policy, >> + err = nlmsg_parse(nlh, sizeof(*nhm), tb, NHA_MAX, rtm_nh_policy_get, >> extack); >> if (err < 0) >> return err; >> >> err = -EINVAL; >> - for (i = 0; i < __NHA_MAX; ++i) { >> - if (!tb[i]) >> - continue; >> - >> - switch (i) { >> - case NHA_ID: >> - break; >> - default: >> - NL_SET_ERR_MSG_ATTR(extack, tb[i], >> - "Unexpected attribute in request"); >> - goto out; >> - } >> - } >> if (nhm->nh_protocol || nhm->resvd || nhm->nh_scope || nhm->nh_flags) { >> NL_SET_ERR_MSG(extack, "Invalid values in header"); >> goto out; >
On Tue, 19 Jan 2021 19:28:40 -0700 David Ahern wrote: > On 1/19/21 1:55 PM, Jakub Kicinski wrote: > >> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c > >> index e53e43aef785..d5d88f7c5c11 100644 > >> --- a/net/ipv4/nexthop.c > >> +++ b/net/ipv4/nexthop.c > >> @@ -36,6 +36,10 @@ static const struct nla_policy rtm_nh_policy[NHA_MAX + 1] = { > >> [NHA_FDB] = { .type = NLA_FLAG }, > >> }; > >> > >> +static const struct nla_policy rtm_nh_policy_get[NHA_MAX + 1] = { > > > > This is an unnecessary waste of memory if you ask me. > > > > NHA_ID is 1, so we're creating an array of 10 extra NULL elements. > > > > Can you leave the size to the compiler and use ARRAY_SIZE() below? > > interesting suggestion in general for netlink attributes. According to tags on commit ff419afa4310 ("ethtool: trim policy tables") the credit goes to Johannes :)
David Ahern <dsahern@gmail.com> writes: > On 1/19/21 1:55 PM, Jakub Kicinski wrote: >>> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c >>> index e53e43aef785..d5d88f7c5c11 100644 >>> --- a/net/ipv4/nexthop.c >>> +++ b/net/ipv4/nexthop.c >>> @@ -36,6 +36,10 @@ static const struct nla_policy rtm_nh_policy[NHA_MAX + 1] = { >>> [NHA_FDB] = { .type = NLA_FLAG }, >>> }; >>> >>> +static const struct nla_policy rtm_nh_policy_get[NHA_MAX + 1] = { >> >> This is an unnecessary waste of memory if you ask me. >> >> NHA_ID is 1, so we're creating an array of 10 extra NULL elements. >> >> Can you leave the size to the compiler and use ARRAY_SIZE() below? > > interesting suggestion in general for netlink attributes. > >> >>> + [NHA_ID] = { .type = NLA_U32 }, >>> +}; >>> + >>> static bool nexthop_notifiers_is_empty(struct net *net) >>> { >>> return !net->nexthop.notifier_chain.head; >>> @@ -1843,27 +1847,14 @@ static int nh_valid_get_del_req(struct nlmsghdr *nlh, u32 *id, >>> { >>> struct nhmsg *nhm = nlmsg_data(nlh); >>> struct nlattr *tb[NHA_MAX + 1]; > > This tb array too could be sized to just the highest indexed expected - > NHA_ID in this case. > >>> - int err, i; >>> + int err; >>> >>> - err = nlmsg_parse(nlh, sizeof(*nhm), tb, NHA_MAX, rtm_nh_policy, >>> + err = nlmsg_parse(nlh, sizeof(*nhm), tb, NHA_MAX, rtm_nh_policy_get, >>> extack); OK, I'll send a v2 that uses ARRAY_SIZE instead of hard-coding the max and size.
Jakub Kicinski <kuba@kernel.org> writes: > On Mon, 18 Jan 2021 15:05:24 +0100 Petr Machata wrote: >> + if (tb[NHA_GROUPS]) >> + *group_filter = true; >> + if (tb[NHA_FDB]) >> + *fdb_filter = true; > > nla_get_flag() OK.
From: Petr Machata <petrm@nvidia.org> There is currently one policy that covers all attributes for next hop object management. Actual validation is then done in code, which makes it unobvious which attributes are acceptable when, and indeed that everything is rejected as necessary. In this series, split rtm_nh_policy to several policies that cover various aspects of the next hop object configuration, and instead of open-coding the validation, defer to nlmsg_parse(). This should make extending the next hop code simpler as well, which will be relevant in near future for resilient hashing implementation. This was tested by running tools/testing/selftests/net/fib_nexthops.sh. Additionally iproute2 was tweaked to issue "nexthop list id" as an RTM_GETNEXTHOP dump request, instead of a straight get to test that unexpected attributes are indeed rejected. In patch #1, convert attribute validation in nh_valid_get_del_req(). In patch #2, convert nh_valid_dump_req(). In patch #3, rtm_nh_policy is cleaned up and renamed to rtm_nh_policy_new, because after the above two patches, that is the only context that it is used in. Petr Machata (3): nexthop: Use a dedicated policy for nh_valid_get_del_req() nexthop: Use a dedicated policy for nh_valid_dump_req() nexthop: Specialize rtm_nh_policy net/ipv4/nexthop.c | 85 +++++++++++++++++----------------------------- 1 file changed, 32 insertions(+), 53 deletions(-)