Date:   Sun, 6 May 2018 19:46:16 -0600
From:   David Ahern <dsa@...ulusnetworks.com>
To:     Roopa Prabhu <roopa@...ulusnetworks.com>, davem@...emloft.net
Cc:     netdev@...r.kernel.org, nikolay@...ulusnetworks.com,
        idosch@...lanox.com
Subject: Re: [PATCH net-next v2 1/3] ipv4: support sport and dport in
 RTM_GETROUTE

On 5/6/18 6:59 PM, Roopa Prabhu wrote:
> From: Roopa Prabhu <roopa@...ulusnetworks.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@...ulusnetworks.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

Powered by blists - more mailing lists