Re: [PATCH net-next v2 1/3] ipv4: support sport and dport in RTM_GETROUTE

On Sun, May 6, 2018 at 6:46 PM, David Ahern [off-list ref] wrote:

On 5/6/18 6:59 PM, Roopa Prabhu wrote:

quoted

From: Roopa Prabhu <redacted>

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 <redacted>
---
 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. ;-)

:) sure..like i mentioned in the cover letter..., i was thinking of
submitting follow up patches for more ip_proto.
since i will be spinning v3, let me see if i can include that too.

I also suggest a comment that these new RTA attributes are used for
GETROUTE only.

sure

And you need to add the new entries to rtm_ipv4_policy.

quoted

      __RTA_MAX
 };

ack,

quoted

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.

hmm..i do see UID returned just 2 lines above. :)

In the least i think it will confirm that the kernel did see your attributes :).

quoted

+
      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;

quoted

+
+     if (p <= 0 || p >= 0xffff)
+             return -EINVAL;

This check is not needed by definition of be16.

ack, will fix the kbuild sparse warning also for both v4 and v6.

quoted

+
+     *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 {}.

yep

quoted

+
+     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

yes

quoted

      }

      /* Reserve room for dummy headers, this skb can pass