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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0f6e29fc-78ce-e0cf-9bde-262ca7430f3d@vyatta.att-mail.com>
Date:   Thu, 20 Sep 2018 17:50:20 +0100
From:   Mike Manning <mmanning@...tta.att-mail.com>
To:     Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Cc:     Dewi Morgan <morgand@...tta.att-mail.com>
Subject: Re: [PATCH net-next 4/5] ipv6: do not drop vrf udp multicast packets

On 20/09/2018 14:02, Paolo Abeni wrote:
> Hi,
>
> On Thu, 2018-09-20 at 09:58 +0100, Mike Manning wrote:
>> diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
>> index 108f5f88ec98..fc60f297d95b 100644
>> --- a/net/ipv6/ip6_input.c
>> +++ b/net/ipv6/ip6_input.c
>> @@ -325,9 +325,12 @@ static int ip6_input_finish(struct net *net, struct sock *sk, struct sk_buff *sk
>>  {
>>  	const struct inet6_protocol *ipprot;
>>  	struct inet6_dev *idev;
>> +	struct net_device *dev;
>>  	unsigned int nhoff;
>> +	int sdif = inet6_sdif(skb);
>>  	int nexthdr;
>>  	bool raw;
>> +	bool deliver;
>>  	bool have_final = false;
> Please, try instead to sort the variable in reverse x-mas tree order.
Will do.
>>  
>>  	/*
>> @@ -371,9 +374,27 @@ 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) {
>> +				rcu_read_lock();
> AFAICS, the rcu lock is already acquired at the beginning of
> ip6_input_finish(), not need to acquire it here again.
Nice catch, I will remove this.
> +				dev = dev_get_by_index_rcu(dev_net(skb->dev),
>> +							   sdif);
>> +				if (!dev) {
>> +					rcu_read_unlock();
>> +					kfree_skb(skb);
>> +					return -ENODEV;
>> +				}
>> +			} else {
>> +				dev = skb->dev;
> The above fragment of code is a recurring pattern in this series,
> perhaps adding an helper for it would reduce code duplication ?

This pattern of checking the secondary device index is used only twice, both in this file.

But with now one instance having the rcu lock handling, and the other not, I cannot refactor this.
>
> Cheers,
>
> Paolo
>
Thanks for the review! I will wait for further comments before producing a v1 of the series.

Regards, Mike