| Message ID | 20231026094106.1505892-3-razor@blackwall.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3de07b963ab81a402a5c5b31e8d2b70c3b08f6ef |
| Delegated to: | BPF |
| Headers | show |
| Series | netkit: two minor cleanups | expand |
On 10/26/23 11:41 AM, Nikolay Aleksandrov wrote: > Use netlink's NLA_POLICY_VALIDATE_FN() type for mode and primary/peer > policy with custom validation functions to return better errors. This > simplifies the logic a bit and relies on netlink's policy validation. > We don't have to specify len because the type is NLA_U32 and attribute > length is enforced by netlink. > > Suggested-by: Jiri Pirko <jiri@resnulli.us> > Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org> Acked-by: Daniel Borkmann <daniel@iogearbox.net> > --- > drivers/net/netkit.c | 66 +++++++++++++++----------------------------- > 1 file changed, 22 insertions(+), 44 deletions(-) Looks better indeed, shrinks code a bit, and passes the selftests, thanks for the suggestion! Thanks, Daniel
Thu, Oct 26, 2023 at 11:41:06AM CEST, razor@blackwall.org wrote: >Use netlink's NLA_POLICY_VALIDATE_FN() type for mode and primary/peer >policy with custom validation functions to return better errors. This >simplifies the logic a bit and relies on netlink's policy validation. >We don't have to specify len because the type is NLA_U32 and attribute >length is enforced by netlink. > >Suggested-by: Jiri Pirko <jiri@resnulli.us> >Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
On Thu, Oct 26, 2023 at 12:41:06PM +0300, Nikolay Aleksandrov wrote: > static const struct nla_policy netkit_policy[IFLA_NETKIT_MAX + 1] = { > [IFLA_NETKIT_PEER_INFO] = { .len = sizeof(struct ifinfomsg) }, > - [IFLA_NETKIT_POLICY] = { .type = NLA_U32 }, > - [IFLA_NETKIT_MODE] = { .type = NLA_U32 }, > - [IFLA_NETKIT_PEER_POLICY] = { .type = NLA_U32 }, > + [IFLA_NETKIT_POLICY] = NLA_POLICY_VALIDATE_FN(NLA_U32, > + netkit_check_policy), Nik, it's problematic to use NLA_POLICY_VALIDATE_FN() with anything other than NLA_BINARY. See commit 9e17f99220d1 ("net/sched: act_mpls: Fix warning during failed attribute validation"). > + [IFLA_NETKIT_MODE] = NLA_POLICY_VALIDATE_FN(NLA_U32, > + netkit_check_mode), > + [IFLA_NETKIT_PEER_POLICY] = NLA_POLICY_VALIDATE_FN(NLA_U32, > + netkit_check_policy), > [IFLA_NETKIT_PRIMARY] = { .type = NLA_REJECT, > .reject_message = "Primary attribute is read-only" }, > }; > -- > 2.38.1 > >
On 10/26/23 17:11, Ido Schimmel wrote: > On Thu, Oct 26, 2023 at 12:41:06PM +0300, Nikolay Aleksandrov wrote: >> static const struct nla_policy netkit_policy[IFLA_NETKIT_MAX + 1] = { >> [IFLA_NETKIT_PEER_INFO] = { .len = sizeof(struct ifinfomsg) }, >> - [IFLA_NETKIT_POLICY] = { .type = NLA_U32 }, >> - [IFLA_NETKIT_MODE] = { .type = NLA_U32 }, >> - [IFLA_NETKIT_PEER_POLICY] = { .type = NLA_U32 }, >> + [IFLA_NETKIT_POLICY] = NLA_POLICY_VALIDATE_FN(NLA_U32, >> + netkit_check_policy), > > Nik, it's problematic to use NLA_POLICY_VALIDATE_FN() with anything > other than NLA_BINARY. See commit 9e17f99220d1 ("net/sched: act_mpls: > Fix warning during failed attribute validation"). > But how is that code called at all? The validation type is NLA_VALIDATE_FUNCTION(), not NLA_VALIDATE_MIN/MAX/RANGE/RANGE_WARN... nla_validate_int_range() is called only on: case NLA_VALIDATE_RANGE_PTR: case NLA_VALIDATE_RANGE: case NLA_VALIDATE_RANGE_WARN_TOO_LONG: case NLA_VALIDATE_MIN: case NLA_VALIDATE_MAX: Anyway, I'll switch to NLA_BINARY in a bit to make sure it's ok. Thanks for the pointer. >> + [IFLA_NETKIT_MODE] = NLA_POLICY_VALIDATE_FN(NLA_U32, >> + netkit_check_mode), >> + [IFLA_NETKIT_PEER_POLICY] = NLA_POLICY_VALIDATE_FN(NLA_U32, >> + netkit_check_policy), >> [IFLA_NETKIT_PRIMARY] = { .type = NLA_REJECT, >> .reject_message = "Primary attribute is read-only" }, >> }; >> -- >> 2.38.1 >> >>
On 10/26/23 4:23 PM, Nikolay Aleksandrov wrote: > On 10/26/23 17:11, Ido Schimmel wrote: >> On Thu, Oct 26, 2023 at 12:41:06PM +0300, Nikolay Aleksandrov wrote: >>> static const struct nla_policy netkit_policy[IFLA_NETKIT_MAX + 1] = { >>> [IFLA_NETKIT_PEER_INFO] = { .len = sizeof(struct ifinfomsg) }, >>> - [IFLA_NETKIT_POLICY] = { .type = NLA_U32 }, >>> - [IFLA_NETKIT_MODE] = { .type = NLA_U32 }, >>> - [IFLA_NETKIT_PEER_POLICY] = { .type = NLA_U32 }, >>> + [IFLA_NETKIT_POLICY] = NLA_POLICY_VALIDATE_FN(NLA_U32, >>> + netkit_check_policy), >> >> Nik, it's problematic to use NLA_POLICY_VALIDATE_FN() with anything >> other than NLA_BINARY. See commit 9e17f99220d1 ("net/sched: act_mpls: >> Fix warning during failed attribute validation"). > > But how is that code called at all? The validation type is NLA_VALIDATE_FUNCTION(), not NLA_VALIDATE_MIN/MAX/RANGE/RANGE_WARN... > nla_validate_int_range() is called only on: > case NLA_VALIDATE_RANGE_PTR: > case NLA_VALIDATE_RANGE: > case NLA_VALIDATE_RANGE_WARN_TOO_LONG: > case NLA_VALIDATE_MIN: > case NLA_VALIDATE_MAX: > > Anyway, I'll switch to NLA_BINARY in a bit to make sure it's ok. Thanks for the pointer. Sg, I took in the NULL removal one for now. Thanks, Daniel
On 10/26/23 17:23, Nikolay Aleksandrov wrote: > On 10/26/23 17:11, Ido Schimmel wrote: >> On Thu, Oct 26, 2023 at 12:41:06PM +0300, Nikolay Aleksandrov wrote: >>> static const struct nla_policy netkit_policy[IFLA_NETKIT_MAX + 1] = { >>> [IFLA_NETKIT_PEER_INFO] = { .len = sizeof(struct >>> ifinfomsg) }, >>> - [IFLA_NETKIT_POLICY] = { .type = NLA_U32 }, >>> - [IFLA_NETKIT_MODE] = { .type = NLA_U32 }, >>> - [IFLA_NETKIT_PEER_POLICY] = { .type = NLA_U32 }, >>> + [IFLA_NETKIT_POLICY] = NLA_POLICY_VALIDATE_FN(NLA_U32, >>> + netkit_check_policy), >> >> Nik, it's problematic to use NLA_POLICY_VALIDATE_FN() with anything >> other than NLA_BINARY. See commit 9e17f99220d1 ("net/sched: act_mpls: >> Fix warning during failed attribute validation"). >> > > But how is that code called at all? The validation type is > NLA_VALIDATE_FUNCTION(), not NLA_VALIDATE_MIN/MAX/RANGE/RANGE_WARN... > nla_validate_int_range() is called only on: > case NLA_VALIDATE_RANGE_PTR: > case NLA_VALIDATE_RANGE: > case NLA_VALIDATE_RANGE_WARN_TOO_LONG: > case NLA_VALIDATE_MIN: > case NLA_VALIDATE_MAX: > Ah, I'm looking at the wrong thing.. I saw the problem. :) > Anyway, I'll switch to NLA_BINARY in a bit to make sure it's ok. Thanks > for the pointer. > >>> + [IFLA_NETKIT_MODE] = NLA_POLICY_VALIDATE_FN(NLA_U32, >>> + netkit_check_mode), >>> + [IFLA_NETKIT_PEER_POLICY] = NLA_POLICY_VALIDATE_FN(NLA_U32, >>> + netkit_check_policy), >>> [IFLA_NETKIT_PRIMARY] = { .type = NLA_REJECT, >>> .reject_message = "Primary attribute is >>> read-only" }, >>> }; >>> -- >>> 2.38.1 >>> >>> >
diff --git a/drivers/net/netkit.c b/drivers/net/netkit.c index 5a0f86f38f09..1ce116e68f95 100644 --- a/drivers/net/netkit.c +++ b/drivers/net/netkit.c @@ -247,29 +247,29 @@ static struct net *netkit_get_link_net(const struct net_device *dev) return peer ? dev_net(peer) : dev_net(dev); } -static int netkit_check_policy(int policy, struct nlattr *tb, +static int netkit_check_policy(const struct nlattr *attr, struct netlink_ext_ack *extack) { - switch (policy) { + switch (nla_get_u32(attr)) { case NETKIT_PASS: case NETKIT_DROP: return 0; default: - NL_SET_ERR_MSG_ATTR(extack, tb, + NL_SET_ERR_MSG_ATTR(extack, attr, "Provided default xmit policy not supported"); return -EINVAL; } } -static int netkit_check_mode(int mode, struct nlattr *tb, +static int netkit_check_mode(const struct nlattr *attr, struct netlink_ext_ack *extack) { - switch (mode) { + switch (nla_get_u32(attr)) { case NETKIT_L2: case NETKIT_L3: return 0; default: - NL_SET_ERR_MSG_ATTR(extack, tb, + NL_SET_ERR_MSG_ATTR(extack, attr, "Provided device mode can only be L2 or L3"); return -EINVAL; } @@ -306,13 +306,8 @@ static int netkit_new_link(struct net *src_net, struct net_device *dev, int err; if (data) { - if (data[IFLA_NETKIT_MODE]) { - attr = data[IFLA_NETKIT_MODE]; - mode = nla_get_u32(attr); - err = netkit_check_mode(mode, attr, extack); - if (err < 0) - return err; - } + if (data[IFLA_NETKIT_MODE]) + mode = nla_get_u32(data[IFLA_NETKIT_MODE]); if (data[IFLA_NETKIT_PEER_INFO]) { attr = data[IFLA_NETKIT_PEER_INFO]; ifmp = nla_data(attr); @@ -324,20 +319,10 @@ static int netkit_new_link(struct net *src_net, struct net_device *dev, return err; tbp = peer_tb; } - if (data[IFLA_NETKIT_POLICY]) { - attr = data[IFLA_NETKIT_POLICY]; - default_prim = nla_get_u32(attr); - err = netkit_check_policy(default_prim, attr, extack); - if (err < 0) - return err; - } - if (data[IFLA_NETKIT_PEER_POLICY]) { - attr = data[IFLA_NETKIT_PEER_POLICY]; - default_peer = nla_get_u32(attr); - err = netkit_check_policy(default_peer, attr, extack); - if (err < 0) - return err; - } + if (data[IFLA_NETKIT_POLICY]) + default_prim = nla_get_u32(data[IFLA_NETKIT_POLICY]); + if (data[IFLA_NETKIT_PEER_POLICY]) + default_peer = nla_get_u32(data[IFLA_NETKIT_PEER_POLICY]); } if (ifmp && tbp[IFLA_IFNAME]) { @@ -818,8 +803,6 @@ static int netkit_change_link(struct net_device *dev, struct nlattr *tb[], struct netkit *nk = netkit_priv(dev); struct net_device *peer = rtnl_dereference(nk->peer); enum netkit_action policy; - struct nlattr *attr; - int err; if (!nk->primary) { NL_SET_ERR_MSG(extack, @@ -834,22 +817,14 @@ static int netkit_change_link(struct net_device *dev, struct nlattr *tb[], } if (data[IFLA_NETKIT_POLICY]) { - attr = data[IFLA_NETKIT_POLICY]; - policy = nla_get_u32(attr); - err = netkit_check_policy(policy, attr, extack); - if (err) - return err; + policy = nla_get_u32(data[IFLA_NETKIT_POLICY]); WRITE_ONCE(nk->policy, policy); } if (data[IFLA_NETKIT_PEER_POLICY]) { - err = -EOPNOTSUPP; - attr = data[IFLA_NETKIT_PEER_POLICY]; - policy = nla_get_u32(attr); - if (peer) - err = netkit_check_policy(policy, attr, extack); - if (err) - return err; + if (!peer) + return -EOPNOTSUPP; + policy = nla_get_u32(data[IFLA_NETKIT_PEER_POLICY]); nk = netkit_priv(peer); WRITE_ONCE(nk->policy, policy); } @@ -889,9 +864,12 @@ static int netkit_fill_info(struct sk_buff *skb, const struct net_device *dev) static const struct nla_policy netkit_policy[IFLA_NETKIT_MAX + 1] = { [IFLA_NETKIT_PEER_INFO] = { .len = sizeof(struct ifinfomsg) }, - [IFLA_NETKIT_POLICY] = { .type = NLA_U32 }, - [IFLA_NETKIT_MODE] = { .type = NLA_U32 }, - [IFLA_NETKIT_PEER_POLICY] = { .type = NLA_U32 }, + [IFLA_NETKIT_POLICY] = NLA_POLICY_VALIDATE_FN(NLA_U32, + netkit_check_policy), + [IFLA_NETKIT_MODE] = NLA_POLICY_VALIDATE_FN(NLA_U32, + netkit_check_mode), + [IFLA_NETKIT_PEER_POLICY] = NLA_POLICY_VALIDATE_FN(NLA_U32, + netkit_check_policy), [IFLA_NETKIT_PRIMARY] = { .type = NLA_REJECT, .reject_message = "Primary attribute is read-only" }, };
Use netlink's NLA_POLICY_VALIDATE_FN() type for mode and primary/peer policy with custom validation functions to return better errors. This simplifies the logic a bit and relies on netlink's policy validation. We don't have to specify len because the type is NLA_U32 and attribute length is enforced by netlink. Suggested-by: Jiri Pirko <jiri@resnulli.us> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org> --- drivers/net/netkit.c | 66 +++++++++++++++----------------------------- 1 file changed, 22 insertions(+), 44 deletions(-)