On 5/6/18 6:59 PM, Roopa Prabhu wrote:
> From: Roopa Prabhu <ro...@cumulusnetworks.com>
>
> This is a followup to fib rules sport, dport match support.
> Having them supported in getroute makes it easier to test
> fib rule lookups. Used by fib rule self tests. Before this patch
> getroute used same skb to pass through the route lookup and
> for the netlink getroute reply msg. This patch allocates separate
> skb's to keep flow dissector happy.
>
> Signed-off-by: Roopa Prabhu <ro...@cumulusnetworks.com>
> ---
> include/uapi/linux/rtnetlink.h | 2 +
> net/ipv4/route.c | 151
> ++++++++++++++++++++++++++++++-----------
> 2 files changed, 115 insertions(+), 38 deletions(-)
>
> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
> index 9b15005..630ecf4 100644
> --- a/include/uapi/linux/rtnetlink.h
> +++ b/include/uapi/linux/rtnetlink.h
> @@ -327,6 +327,8 @@ enum rtattr_type_t {
> RTA_PAD,
> RTA_UID,
> RTA_TTL_PROPAGATE,
> + RTA_SPORT,
> + RTA_DPORT,
If you are going to add sport and dport because of the potential for FIB
rules, you need to add ip-proto as well. I realize existing code assumed
UDP, but the FIB rules cover any IP proto. Yes, I know this makes the
change much larger to generate tcp, udp as well as iphdr options; the
joys of new features. ;-)
I also suggest a comment that these new RTA attributes are used for
GETROUTE only.
And you need to add the new entries to rtm_ipv4_policy.
> __RTA_MAX
> };
>
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 1412a7b..e91ed62 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -2568,11 +2568,10 @@ struct rtable *ip_route_output_flow(struct net *net,
> struct flowi4 *flp4,
> EXPORT_SYMBOL_GPL(ip_route_output_flow);
>
> /* called with rcu_read_lock held */
> -static int rt_fill_info(struct net *net, __be32 dst, __be32 src, u32
> table_id,
> - struct flowi4 *fl4, struct sk_buff *skb, u32 portid,
> - u32 seq)
> +static int rt_fill_info(struct net *net, __be32 dst, __be32 src,
> + struct rtable *rt, u32 table_id, struct flowi4 *fl4,
> + struct sk_buff *skb, u32 portid, u32 seq)
> {
> - struct rtable *rt = skb_rtable(skb);
> struct rtmsg *r;
> struct nlmsghdr *nlh;
> unsigned long expires = 0;
> @@ -2651,6 +2650,14 @@ static int rt_fill_info(struct net *net, __be32 dst,
> __be32 src, u32 table_id,
> from_kuid_munged(current_user_ns(), fl4->flowi4_uid)))
> goto nla_put_failure;
>
> + if (fl4->fl4_sport &&
> + nla_put_be16(skb, RTA_SPORT, fl4->fl4_sport))
> + goto nla_put_failure;
> +
> + if (fl4->fl4_dport &&
> + nla_put_be16(skb, RTA_DPORT, fl4->fl4_dport))
> + goto nla_put_failure;
Why return the attributes to the user? I can't see any value in that.
UID option is not returned either so there is precedence.
> +
> error = rt->dst.error;
>
> if (rt_is_input_route(rt)) {
> @@ -2668,7 +2675,7 @@ static int rt_fill_info(struct net *net, __be32 dst,
> __be32 src, u32 table_id,
> }
> } else
> #endif
> - if (nla_put_u32(skb, RTA_IIF, skb->dev->ifindex))
> + if (nla_put_u32(skb, RTA_IIF, fl4->flowi4_iif))
> goto nla_put_failure;
> }
>
> @@ -2683,35 +2690,86 @@ static int rt_fill_info(struct net *net, __be32 dst,
> __be32 src, u32 table_id,
> return -EMSGSIZE;
> }
>
> +static int nla_get_port(struct nlattr *attr, __be16 *port)
> +{
> + int p = nla_get_be16(attr);
__be16 p;
> +
> + if (p <= 0 || p >= 0xffff)
> + return -EINVAL;
This check is not needed by definition of be16.
> +
> + *port = p;
> + return 0;
> +}
> +
> +static int inet_rtm_getroute_reply(struct sk_buff *in_skb, struct nlmsghdr
> *nlh,
> + __be32 dst, __be32 src, struct flowi4 *fl4,
> + struct rtable *rt, struct fib_result *res)
> +{
> + struct net *net = sock_net(in_skb->sk);
> + struct rtmsg *rtm = nlmsg_data(nlh);
> + u32 table_id = RT_TABLE_MAIN;
> + struct sk_buff *skb;
> + int err = 0;
> +
> + skb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
> + if (!skb) {
> + err = -ENOMEM;
> + return err;
> + }
just 'return -ENOMEM' and without the {}.
> +
> + if (rtm->rtm_flags & RTM_F_LOOKUP_TABLE)
> + table_id = res->table ? res->table->tb_id : 0;
> +
> + if (rtm->rtm_flags & RTM_F_FIB_MATCH)
> + err = fib_dump_info(skb, NETLINK_CB(in_skb).portid,
> + nlh->nlmsg_seq, RTM_NEWROUTE, table_id,
> + rt->rt_type, res->prefix, res->prefixlen,
> + fl4->flowi4_tos, res->fi, 0);
> + else
> + err = rt_fill_info(net, dst, src, rt, table_id,
> + fl4, skb, NETLINK_CB(in_skb).portid,
> + nlh->nlmsg_seq);
> + if (err < 0)
> + goto errout;
> +
> + return rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid);
> +
> +errout:
> + kfree_skb(skb);
> + return err;
> +}
> +
> static int inet_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
> struct netlink_ext_ack *extack)
> {
> struct net *net = sock_net(in_skb->sk);
> - struct rtmsg *rtm;
> struct nlattr *tb[RTA_MAX+1];
> + __be16 sport = 0, dport = 0;
> struct fib_result res = {};
> struct rtable *rt = NULL;
> + struct sk_buff *skb;
> + struct rtmsg *rtm;
> struct flowi4 fl4;
> + struct iphdr *iph;
> + struct udphdr *udph;
> __be32 dst = 0;
> __be32 src = 0;
> + kuid_t uid;
> u32 iif;
> int err;
> int mark;
> - struct sk_buff *skb;
> - u32 table_id = RT_TABLE_MAIN;
> - kuid_t uid;
>
> err = nlmsg_parse(nlh, sizeof(*rtm), tb, RTA_MAX, rtm_ipv4_policy,
> extack);
> if (err < 0)
> - goto errout;
> + return err;
>
> rtm = nlmsg_data(nlh);
>
> skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
> if (!skb) {
> err = -ENOBUFS;
> - goto errout;
> + return err;
just return -ENOBUFS
> }
>
> /* Reserve room for dummy headers, this skb can pass
