netdev - Packet gets stuck in NOLOCK pfifo

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

Message-ID: <d102074f-7489-e35a-98cf-e2cad7efd8a2@netrounds.com>
Date:   Wed, 9 Oct 2019 08:46:19 +0200
From:   Jonas Bonn <jonas.bonn@...rounds.com>
To:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "David S . Miller" <davem@...emloft.net>, pabeni@...hat.com
Subject: Packet gets stuck in NOLOCK pfifo_fast qdisc

Hi,

The lockless pfifo_fast qdisc has an issue with packets getting stuck in 
the queue.  What appears to happen is:

i)  Thread 1 holds the 'seqlock' on the qdisc and dequeues packets.
ii)  Thread 1 dequeues the last packet in the queue.
iii)  Thread 1 iterates through the qdisc->dequeue function again and 
determines that the queue is empty.

iv)  Thread 2 queues up a packet.  Since 'seqlock' is busy, it just 
assumes the packet will be dequeued by whoever is holding the lock.

v)  Thread 1 releases 'seqlock'.

After v), nobody will check if there are packets in the queue until a 
new packet is enqueued.  Thereby, the packet enqueued by Thread 2 may be 
delayed indefinitely.

What, I think, should probably happen is that Thread 1 should check that 
the queue is empty again after releasing 'seqlock'.  I poked at this, 
but it wasn't obvious to me how to go about this given the way the 
layering works here.  Roughly:

qdisc_run_end() {
...
	spin_unlock(seqlock);
	if (!qdisc_is_empty(qdisc))
		qdisc_run();
...
}

Calling qdisc_run() from qdisc_run_end() doesn't feel right!

There's a qdisc->empty property (and qdisc_is_empty() relies on it) but 
it's not particularly useful in this case since there's a race in 
setting this property which makes it not quite reliable.

Hope someone can shine some light on how to proceed here.

/Jonas