linux-kernel - Re: [PATCH] xfrm : lock input tasklet skb queue

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: <CACVy4SV3K257XfFkR_ahkU2yy9mzJD-9LrSiQPCnespB3k_0XQ@mail.gmail.com>
Date:   Mon, 21 Oct 2019 09:31:13 -0700
From:   Tom Rix <trix@...hat.com>
To:     Steffen Klassert <steffen.klassert@...unet.com>
Cc:     herbert@...dor.apana.org.au, davem@...emloft.net,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] xfrm : lock input tasklet skb queue

When preempt rt is full, softirq and interrupts run in kthreads. So it
is possible for the tasklet to sleep and for its queue to get modified
while it sleeps.

On Mon, Oct 21, 2019 at 1:37 AM Steffen Klassert
<steffen.klassert@...unet.com> wrote:
>
> On Sun, Oct 20, 2019 at 08:46:10AM -0700, Tom Rix wrote:
> > On PREEMPT_RT_FULL while running netperf, a corruption
> > of the skb queue causes an oops.
> >
> > This appears to be caused by a race condition here
> >         __skb_queue_tail(&trans->queue, skb);
> >         tasklet_schedule(&trans->tasklet);
> > Where the queue is changed before the tasklet is locked by
> > tasklet_schedule.
> >
> > The fix is to use the skb queue lock.
> >
> > Signed-off-by: Tom Rix <trix@...hat.com>
> > ---
> >  net/xfrm/xfrm_input.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> > index 9b599ed66d97..226dead86828 100644
> > --- a/net/xfrm/xfrm_input.c
> > +++ b/net/xfrm/xfrm_input.c
> > @@ -758,12 +758,16 @@ static void xfrm_trans_reinject(unsigned long data)
> >      struct xfrm_trans_tasklet *trans = (void *)data;
> >      struct sk_buff_head queue;
> >      struct sk_buff *skb;
> > +    unsigned long flags;
> >
> >      __skb_queue_head_init(&queue);
> > +    spin_lock_irqsave(&trans->queue.lock, flags);
> >      skb_queue_splice_init(&trans->queue, &queue);
> >
> >      while ((skb = __skb_dequeue(&queue)))
> >          XFRM_TRANS_SKB_CB(skb)->finish(dev_net(skb->dev), NULL, skb);
> > +
> > +    spin_unlock_irqrestore(&trans->queue.lock, flags);
> >  }
> >
> >  int xfrm_trans_queue(struct sk_buff *skb,
> > @@ -771,15 +775,20 @@ int xfrm_trans_queue(struct sk_buff *skb,
> >                     struct sk_buff *))
> >  {
> >      struct xfrm_trans_tasklet *trans;
> > +    unsigned long flags;
> >
> >      trans = this_cpu_ptr(&xfrm_trans_tasklet);
> > +    spin_lock_irqsave(&trans->queue.lock, flags);
>
> As you can see above 'trans' is per cpu, so a spinlock
> is not needed here. Also this does not run in hard
> interrupt context, so irqsave is also not needed.
> I don't see how this can fix anything.
>
> Can you please explain that race a bit more detailed?
>