From mboxrd@z Thu Jan  1 00:00:00 1970
From: Mike Manning <mmanning@vyatta.att-mail.com>
Subject: Re: [PATCH net-next 4/5] ipv6: do not drop vrf udp multicast packets
Date: Thu, 20 Sep 2018 17:50:20 +0100
Message-ID: <0f6e29fc-78ce-e0cf-9bde-262ca7430f3d@vyatta.att-mail.com>
References: <20180920085848.17721-1-mmanning@vyatta.att-mail.com>
 <20180920085848.17721-5-mmanning@vyatta.att-mail.com>
 <09797df92f76885b0e3244a3aca68dbf07a09a3a.camel@redhat.com>
Reply-To: mmanning@vyatta.att-mail.com
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Cc: Dewi Morgan <morgand@vyatta.att-mail.com>
To: Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org
Return-path: <netdev-owner@vger.kernel.org>
Received: from mx0b-00191d01.pphosted.com ([67.231.157.136]:36390 "EHLO
        mx0a-00191d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL)
        by vger.kernel.org with ESMTP id S1728016AbeITWfD (ORCPT
        <rfc822;netdev@vger.kernel.org>); Thu, 20 Sep 2018 18:35:03 -0400
In-Reply-To: <09797df92f76885b0e3244a3aca68dbf07a09a3a.camel@redhat.com>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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