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
