Re: [PATCH net-next v1 4/5] ipv6: do not drop vrf udp multicast packets

[David Ahern <dsahern@gmail.com>]

On 9/24/18 10:13 AM, Mike Manning wrote:
> From: Dewi Morgan <morgand@vyatta.att-mail.com>
> 
> For bound udp sockets in a vrf, also check the sdif to get the index
> for ingress devices enslaved to an l3mdev. Verify the multicast address
> against the enslaved rather than the l3mdev device.
> 
> Signed-off-by: Dewi Morgan <morgand@vyatta.att-mail.com>
> Signed-off-by: Mike Manning <mmanning@vyatta.att-mail.com>
> ---
>  net/ipv6/ip6_input.c | 24 ++++++++++++++++++++----
>  net/ipv6/udp.c       |  8 +++++---
>  2 files changed, 25 insertions(+), 7 deletions(-)
> 

This should be 2 patches -- 1 that modifies the socket lookup to
consider and 1 that alters in the input path. They are completely
separate changes.


> diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
> index 108f5f88ec98..82ffb5cdd2ab 100644
> --- a/net/ipv6/ip6_input.c
> +++ b/net/ipv6/ip6_input.c
> @@ -324,11 +324,14 @@ void ipv6_list_rcv(struct list_head *head, struct packet_type *pt,
>  static int ip6_input_finish(struct net *net, struct sock *sk, struct sk_buff *skb)
>  {
>  	const struct inet6_protocol *ipprot;
> +	int sdif = inet6_sdif(skb);
> +	bool have_final = false;
>  	struct inet6_dev *idev;
> +	struct net_device *dev;

make sdif and dev declarations local to where they are needed.


>  	unsigned int nhoff;
> +	bool deliver;

deliver is not needed.

>  	int nexthdr;
>  	bool raw;
> -	bool have_final = false;

so no need to move this one.

>  
>  	/*
>  	 *	Parse extension headers
> @@ -371,9 +374,22 @@ static int ip6_input_finish(struct net *net, struct sock *sk, struct sk_buff *sk
>  			skb_postpull_rcsum(skb, skb_network_header(skb),
>  					   skb_network_header_len(skb));
>  			hdr = ipv6_hdr(skb);
> -			if (ipv6_addr_is_multicast(&hdr->daddr) &&
> -			    !ipv6_chk_mcast_addr(skb->dev, &hdr->daddr,
> -			    &hdr->saddr) &&
> +
> +			/* skb->dev passed may be master dev for vrfs. */
> +			if (sdif) {
> +				dev = dev_get_by_index_rcu(dev_net(skb->dev),

net is a passed in argument. Why not use it?

> +							   sdif);
> +				if (!dev) {
> +					kfree_skb(skb);
> +					return -ENODEV;

The rcu_read_lock() is held. I believe 'goto discard' is sufficient if
the enslaved device disappeared.

> +				}
> +			} else {
> +				dev = skb->dev;
> +			}
> +
> +			deliver = ipv6_chk_mcast_addr(dev, &hdr->daddr,
> +						      &hdr->saddr);
> +			if (ipv6_addr_is_multicast(&hdr->daddr) && !deliver &&
>  			    !ipv6_is_mld(skb, nexthdr, skb_network_header_len(skb)))
>  				goto discard;
>  		}

I think the original code only needs skb->dev changed to dev making this
a much smaller patch.