netdev - Re: [PATCH 4.14 stable v2 1/5] ipv6: frags: fix a lockdep false positive

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] [day] [month] [year] [list]

Date:   Fri, 26 Apr 2019 13:28:44 -0600
From:   Captain Wiggum <captwiggum@...il.com>
To:     Peter Oskolkov <posk@...gle.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        stable@...r.kernel.org, netdev@...r.kernel.org,
        Peter Oskolkov <posk@...k.io>,
        David Miller <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Sasha Levin <sashal@...nel.org>, Lars Persson <lists@...h.nu>
Subject: Re: [PATCH 4.14 stable v2 1/5] ipv6: frags: fix a lockdep false positive

Sorry for the late reply.
I am starting the TAHI test on the 4.14 v2 patch today and reply with
results before Mon morning.
The test is long running so we let it run overnight.

On Tue, Apr 23, 2019 at 11:48 AM Peter Oskolkov <posk@...gle.com> wrote:
>
> From: Eric Dumazet <edumazet@...gle.com>
>
> [ Upstream commit 415787d7799f4fccbe8d49cb0b8e5811be6b0389 ]
>
> lockdep does not know that the locks used by IPv4 defrag
> and IPv6 reassembly units are of different classes.
>
> It complains because of following chains :
>
> 1) sch_direct_xmit()        (lock txq->_xmit_lock)
>     dev_hard_start_xmit()
>      xmit_one()
>       dev_queue_xmit_nit()
>        packet_rcv_fanout()
>         ip_check_defrag()
>          ip_defrag()
>           spin_lock()     (lock frag queue spinlock)
>
> 2) ip6_input_finish()
>     ipv6_frag_rcv()       (lock frag queue spinlock)
>      ip6_frag_queue()
>       icmpv6_param_prob() (lock txq->_xmit_lock at some point)
>
> We could add lockdep annotations, but we also can make sure IPv6
> calls icmpv6_param_prob() only after the release of the frag queue spinlock,
> since this naturally makes frag queue spinlock a leaf in lock hierarchy.
>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Signed-off-by: David S. Miller <davem@...emloft.net>
> ---
>  net/ipv6/reassembly.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
> index 2a8c680b67cd..f75e9e711c31 100644
> --- a/net/ipv6/reassembly.c
> +++ b/net/ipv6/reassembly.c
> @@ -170,7 +170,8 @@ fq_find(struct net *net, __be32 id, const struct ipv6hdr *hdr, int iif)
>  }
>
>  static int ip6_frag_queue(struct frag_queue *fq, struct sk_buff *skb,
> -                          struct frag_hdr *fhdr, int nhoff)
> +                         struct frag_hdr *fhdr, int nhoff,
> +                         u32 *prob_offset)
>  {
>         struct sk_buff *prev, *next;
>         struct net_device *dev;
> @@ -186,11 +187,7 @@ static int ip6_frag_queue(struct frag_queue *fq, struct sk_buff *skb,
>                         ((u8 *)(fhdr + 1) - (u8 *)(ipv6_hdr(skb) + 1)));
>
>         if ((unsigned int)end > IPV6_MAXPLEN) {
> -               __IP6_INC_STATS(net, ip6_dst_idev(skb_dst(skb)),
> -                               IPSTATS_MIB_INHDRERRORS);
> -               icmpv6_param_prob(skb, ICMPV6_HDR_FIELD,
> -                                 ((u8 *)&fhdr->frag_off -
> -                                  skb_network_header(skb)));
> +               *prob_offset = (u8 *)&fhdr->frag_off - skb_network_header(skb);
>                 return -1;
>         }
>
> @@ -221,10 +218,7 @@ static int ip6_frag_queue(struct frag_queue *fq, struct sk_buff *skb,
>                         /* RFC2460 says always send parameter problem in
>                          * this case. -DaveM
>                          */
> -                       __IP6_INC_STATS(net, ip6_dst_idev(skb_dst(skb)),
> -                                       IPSTATS_MIB_INHDRERRORS);
> -                       icmpv6_param_prob(skb, ICMPV6_HDR_FIELD,
> -                                         offsetof(struct ipv6hdr, payload_len));
> +                       *prob_offset = offsetof(struct ipv6hdr, payload_len);
>                         return -1;
>                 }
>                 if (end > fq->q.len) {
> @@ -536,15 +530,22 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
>         iif = skb->dev ? skb->dev->ifindex : 0;
>         fq = fq_find(net, fhdr->identification, hdr, iif);
>         if (fq) {
> +               u32 prob_offset = 0;
>                 int ret;
>
>                 spin_lock(&fq->q.lock);
>
>                 fq->iif = iif;
> -               ret = ip6_frag_queue(fq, skb, fhdr, IP6CB(skb)->nhoff);
> +               ret = ip6_frag_queue(fq, skb, fhdr, IP6CB(skb)->nhoff,
> +                                    &prob_offset);
>
>                 spin_unlock(&fq->q.lock);
>                 inet_frag_put(&fq->q);
> +               if (prob_offset) {
> +                       __IP6_INC_STATS(net, ip6_dst_idev(skb_dst(skb)),
> +                                       IPSTATS_MIB_INHDRERRORS);
> +                       icmpv6_param_prob(skb, ICMPV6_HDR_FIELD, prob_offset);
> +               }
>                 return ret;
>         }
>
> --
> 2.21.0.593.g511ec345e18-goog
>