On Sun, May 6, 2018 at 6:46 PM, David Ahern <d...@cumulusnetworks.com> wrote:
> 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. ;-)
:) 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
>
