| Message ID | 20220707093336.214658-1-equinox@diac24.net (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v5] net: ip6mr: add RTM_GETROUTE netlink op | expand |
Few more nit picks, sorry.. On Thu, 7 Jul 2022 11:33:36 +0200 David Lamparter wrote: > +static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh, > + struct netlink_ext_ack *extack) > +{ > + struct net *net = sock_net(in_skb->sk); > + struct in6_addr src = {}, grp = {}; > + struct nlattr *tb[RTA_MAX + 1]; > + struct sk_buff *skb = NULL; Should be unnecessary if the code is right, let the compiler warn us about uninitialized variables. > + struct mfc6_cache *cache; > + struct mr_table *mrt; > + u32 tableid; > + int err; > + > + err = ip6mr_rtm_valid_getroute_req(in_skb, nlh, tb, extack); > + if (err < 0) > + goto errout; Can we: return err; ? I don't know where the preference for jumping to the return statement came from, old compilers? someone's "gut feeling"? > + if (tb[RTA_SRC]) > + src = nla_get_in6_addr(tb[RTA_SRC]); > + if (tb[RTA_DST]) > + grp = nla_get_in6_addr(tb[RTA_DST]); > + tableid = tb[RTA_TABLE] ? nla_get_u32(tb[RTA_TABLE]) : 0; > + > + mrt = ip6mr_get_table(net, tableid ? tableid : RT_TABLE_DEFAULT); tableid ? : RT_TABLE_DEFAULT or tableid ?: RT_TABLE_DEFAULT the abbreviated version of the ternary operator is used quite commonly in the kernel. > + if (!mrt) { > + NL_SET_ERR_MSG_MOD(extack, "MR table does not exist"); > + err = -ENOENT; > + goto errout_free; Ditto, just return, if not goto errout; there's nothing to free. > + } > + > + /* entries are added/deleted only under RTNL */ > + rcu_read_lock(); > + cache = ip6mr_cache_find(mrt, &src, &grp); > + rcu_read_unlock(); > + if (!cache) { > + NL_SET_ERR_MSG_MOD(extack, "MR cache entry not found"); > + err = -ENOENT; > + goto errout_free; > + } > + > + skb = nlmsg_new(mr6_msgsize(false, mrt->maxvif), GFP_KERNEL); > + if (!skb) { > + err = -ENOBUFS; > + goto errout_free; > + } > + > + err = ip6mr_fill_mroute(mrt, skb, NETLINK_CB(in_skb).portid, > + nlh->nlmsg_seq, cache, RTM_NEWROUTE, 0); > + if (err < 0) > + goto errout_free; now this is the only case which actually needs to free the skb > + > + err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid); > + > +errout: > + return err; when the label is gone you can: return rtnl_unicast(... directly. > + > +errout_free: > + kfree_skb(skb); > + goto errout; and no need to do the funky backwards jump here either, IMO > +}
On Fri, Jul 08, 2022 at 08:29:51PM -0700, Jakub Kicinski wrote: > Few more nit picks, sorry.. Thanks for the feedback! [opens editor] > On Thu, 7 Jul 2022 11:33:36 +0200 David Lamparter wrote: [...] > > + err = ip6mr_rtm_valid_getroute_req(in_skb, nlh, tb, extack); > > + if (err < 0) > > + goto errout; > > Can we: > > return err; > > ? I don't know where the preference for jumping to the return statement > came from, old compilers? someone's "gut feeling"? If I were forced to find a justification, I'd say having a central sequence of exit helps avoiding mistakes when some other resource acquisition is added later. Easy to add a cleanup call to an existing cleanup block - easy to overlook a "return err;" that needs to be changed to "goto errout;". But I have absolutely no stake in this at all, I'll happily edit it to whatever the consensus is. This is just what the IPv4 code looks like after being adapted for IPv6. > > +errout: > > + return err; [...] > > + > > +errout_free: > > + kfree_skb(skb); > > + goto errout; > > and no need to do the funky backwards jump here either, IMO "funky" is a nice description. -equi/David
On Sat, 9 Jul 2022 14:45:00 +0200 David Lamparter wrote: > > > + err = ip6mr_rtm_valid_getroute_req(in_skb, nlh, tb, extack); > > > + if (err < 0) > > > + goto errout; > > > > Can we: > > > > return err; > > > > ? I don't know where the preference for jumping to the return statement > > came from, old compilers? someone's "gut feeling"? > > If I were forced to find a justification, I'd say having a central > sequence of exit helps avoiding mistakes when some other resource > acquisition is added later. Easy to add a cleanup call to an existing > cleanup block - easy to overlook a "return err;" that needs to be > changed to "goto errout;". That only works if the label's name is meaningless, if the label is named after what it points to you have to rename the label and all the jumps anyway. Can as well replace returns with a goto. > But I have absolutely no stake in this at all, I'll happily edit it to > whatever the consensus is. This is just what the IPv4 code looks like > after being adapted for IPv6. Ah, I looked around other getroute implementations but not specifically ipmr. I'd rather refactor ipmr.c as well than keep its strangeness. The fact that we jump to the error path which tries to free the skb without ever allocating the skb feels particularly off.
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c index ec6e1509fc7c..4aee3d8a6103 100644 --- a/net/ipv6/ip6mr.c +++ b/net/ipv6/ip6mr.c @@ -95,6 +95,8 @@ static int ip6mr_cache_report(const struct mr_table *mrt, struct sk_buff *pkt, static void mr6_netlink_event(struct mr_table *mrt, struct mfc6_cache *mfc, int cmd); static void mrt6msg_netlink_event(const struct mr_table *mrt, struct sk_buff *pkt); +static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh, + struct netlink_ext_ack *extack); static int ip6mr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb); static void mroute_clean_tables(struct mr_table *mrt, int flags); @@ -1390,7 +1392,7 @@ int __init ip6_mr_init(void) } #endif err = rtnl_register_module(THIS_MODULE, RTNL_FAMILY_IP6MR, RTM_GETROUTE, - NULL, ip6mr_rtm_dumproute, 0); + ip6mr_rtm_getroute, ip6mr_rtm_dumproute, 0); if (err == 0) return 0; @@ -2510,6 +2512,104 @@ static void mrt6msg_netlink_event(const struct mr_table *mrt, struct sk_buff *pk rtnl_set_sk_err(net, RTNLGRP_IPV6_MROUTE_R, -ENOBUFS); } +static const struct nla_policy ip6mr_getroute_policy[RTA_MAX + 1] = { + [RTA_SRC] = NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)), + [RTA_DST] = NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)), + [RTA_TABLE] = { .type = NLA_U32 }, +}; + +static int ip6mr_rtm_valid_getroute_req(struct sk_buff *skb, + const struct nlmsghdr *nlh, + struct nlattr **tb, + struct netlink_ext_ack *extack) +{ + struct rtmsg *rtm; + int err; + + err = nlmsg_parse(nlh, sizeof(*rtm), tb, RTA_MAX, ip6mr_getroute_policy, + extack); + if (err) + return err; + + rtm = nlmsg_data(nlh); + if ((rtm->rtm_src_len && rtm->rtm_src_len != 128) || + (rtm->rtm_dst_len && rtm->rtm_dst_len != 128) || + rtm->rtm_tos || rtm->rtm_table || rtm->rtm_protocol || + rtm->rtm_scope || rtm->rtm_type || rtm->rtm_flags) { + NL_SET_ERR_MSG_MOD(extack, + "Invalid values in header for multicast route get request"); + return -EINVAL; + } + + if ((tb[RTA_SRC] && !rtm->rtm_src_len) || + (tb[RTA_DST] && !rtm->rtm_dst_len)) { + NL_SET_ERR_MSG_MOD(extack, "rtm_src_len and rtm_dst_len must be 128 for IPv6"); + return -EINVAL; + } + + return 0; +} + +static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh, + struct netlink_ext_ack *extack) +{ + struct net *net = sock_net(in_skb->sk); + struct in6_addr src = {}, grp = {}; + struct nlattr *tb[RTA_MAX + 1]; + struct sk_buff *skb = NULL; + struct mfc6_cache *cache; + struct mr_table *mrt; + u32 tableid; + int err; + + err = ip6mr_rtm_valid_getroute_req(in_skb, nlh, tb, extack); + if (err < 0) + goto errout; + + if (tb[RTA_SRC]) + src = nla_get_in6_addr(tb[RTA_SRC]); + if (tb[RTA_DST]) + grp = nla_get_in6_addr(tb[RTA_DST]); + tableid = tb[RTA_TABLE] ? nla_get_u32(tb[RTA_TABLE]) : 0; + + mrt = ip6mr_get_table(net, tableid ? tableid : RT_TABLE_DEFAULT); + if (!mrt) { + NL_SET_ERR_MSG_MOD(extack, "MR table does not exist"); + err = -ENOENT; + goto errout_free; + } + + /* entries are added/deleted only under RTNL */ + rcu_read_lock(); + cache = ip6mr_cache_find(mrt, &src, &grp); + rcu_read_unlock(); + if (!cache) { + NL_SET_ERR_MSG_MOD(extack, "MR cache entry not found"); + err = -ENOENT; + goto errout_free; + } + + skb = nlmsg_new(mr6_msgsize(false, mrt->maxvif), GFP_KERNEL); + if (!skb) { + err = -ENOBUFS; + goto errout_free; + } + + err = ip6mr_fill_mroute(mrt, skb, NETLINK_CB(in_skb).portid, + nlh->nlmsg_seq, cache, RTM_NEWROUTE, 0); + if (err < 0) + goto errout_free; + + err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid); + +errout: + return err; + +errout_free: + kfree_skb(skb); + goto errout; +} + static int ip6mr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb) { const struct nlmsghdr *nlh = cb->nlh;