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

[Roopa Prabhu <roopa@cumulusnetworks.com>]

On Sun, May 6, 2018 at 6:46 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
> On 5/6/18 6:59 PM, Roopa Prabhu wrote:
>> From: Roopa Prabhu <roopa@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 <roopa@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. ;-)


:) 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.
>
>
>>       __RTA_MAX
>>  };

ack,

>>
>> 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 :).


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

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


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

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

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