Re: [PATCH] nfc: rawsock: prevent TX work requeue after socket shutdown

[zhang san]

Hello,

I am resending my response in plain text because my earlier Gmail
reply contained an HTML part and was rejected by the netdev,
linux-kernel, and oe-linux-nfc mailing lists.

The issue was identified during AI-assisted source-code review and
investigated with a KASAN-enabled kernel and a deterministic
timing-injection harness using a virtual NFC device in QEMU. No
physical NFC hardware was used.

The High-severity finding on v1 is valid. rawsock_release() set
SEND_SHUTDOWN without holding sk_write_queue.lock, allowing a delayed
completion to requeue tx_work after cancel_work_sync() had returned.
In v2, release sets SEND_SHUTDOWN under the same lock used by
rawsock_data_exchange_complete() for the shutdown check and
schedule_work().

The Low-severity Fixes-tag finding is also valid. V2 uses Fixes:
d793458c45df ("nfc: rawsock: cancel tx_work before socket teardown").

The timing-injection test produced seven slab-use-after-free reports
with the v1 locking and none with the revised locking under the same
conditions. We do not claim that this patch eliminates all potential
data races involving sk_shutdown.

V2 was already sent as a separate thread:
https://lore.kernel.org/r/20261105113812.969651-1-shunsheng1218@xxxxxxxxx/

Best regards,
Yaohan Li

pw-bot: changes-requested

On Thu, 05 Nov 2026 09:58:39 +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 0 · Low: 1
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] The fix is still racy, so the use-after-free it claims to close
> remains reachable.
> - [Low] The patch only works on top of commit d793458c45df2a ("nfc:
> rawsock: cancel tx_work before socket teardown", 2026-03-03), and the…
>
> --
>
> nfc: rawsock: prevent TX work requeue after socket shutdown
>
> The patch makes rawsock_data_exchange_complete() check SEND_SHUTDOWN
> under sk_write_queue.lock before it reschedules tx_work. The goal is to
> stop a delayed data-exchange completion from requeueing the work after
> rawsock_release() has started tearing the socket down.
>
> > diff --git a/net/nfc/rawsock.c b/net/nfc/rawsock.c
> > index f7d7a599fade7..efdbd512d4600 100644
> > --- a/net/nfc/rawsock.c
> > +++ b/net/nfc/rawsock.c
> > @@ -168,7 +168,13 @@ static void rawsock_data_exchange_complete(void *context, struct sk_buff *skb,
> > goto error_skb;
> >
> > spin_lock_bh(&sk->sk_write_queue.lock);
> > - if (!skb_queue_empty(&sk->sk_write_queue))
> > + /*
> > + * The release path sets SEND_SHUTDOWN before cancel_work_sync():
> > + * once it is set, do not requeue tx_work - the socket may be freed
> > + * right after the cancel returns.
> > + */
>
> [Severity: Low]
> This comment relies on the SEND_SHUTDOWN / cancel_work_sync() block in
> rawsock_release(). That block was added by commit d793458c45df2a ("nfc:
> rawsock: cancel tx_work before socket teardown"). Before that commit,
> rawsock_release() did not set SEND_SHUTDOWN and did not cancel tx_work.
>
> The patch only carries:
>
> Fixes: 23b7869c0fd08 ("NFC: add the NFC socket raw protocol")
>
> The lines it changes date back to 2011. It would apply cleanly to a
> stable tree that lacks d793458c45df2a, and there the new check would do
> nothing during release.
>
> Should this have a Fixes: tag for d793458c45df ("nfc: rawsock: cancel
> tx_work before socket teardown"), or at least name it as a prerequisite?
>
> > + if (!skb_queue_empty(&sk->sk_write_queue) &&
> > + !(sk->sk_shutdown & SEND_SHUTDOWN))
> > schedule_work(&nfc_rawsock(sk)->tx_work);
>
> [Severity: High]
> Is this check actually serialized with the release path? Only the
> completion side takes sk_write_queue.lock here. rawsock_release() sets
> the flag and cancels the work without holding that lock:
>
> net/nfc/rawsock.c:rawsock_release() {
> ...
> if (sk->sk_state == TCP_ESTABLISHED) {
> ...
> sk->sk_shutdown |= SEND_SHUTDOWN;
> cancel_work_sync(&nfc_rawsock(sk)->tx_work);
> rawsock_write_queue_purge(sk);
> }
>
> sock_orphan(sk);
> sock_put(sk);
> ...
> }
>
> Release only takes the lock later, inside rawsock_write_queue_purge().
>
> Can the following interleaving still happen? CPU0 holds the reference
> taken by sock_hold() in rawsock_tx_work():
>
> CPU0 (rawsock_data_exchange_complete) CPU1 (rawsock_release)
> spin_lock_bh(&sk->sk_write_queue.lock)
> queue non-empty, sk_shutdown == 0
> sk->sk_shutdown |= SEND_SHUTDOWN
> cancel_work_sync(&tx_work)
> tx_work idle, returns at once
> schedule_work(&tx_work) succeeds
> spin_unlock_bh()
> sock_put(sk)
> rawsock_write_queue_purge(sk)
> sock_orphan(sk)
> sock_put(sk)
> last ref, socket freed
>
> If tx_work is neither pending nor running, cancel_work_sync() returns
> right away. __cancel_work_sync() also calls enable_work() before it
> returns, so the later schedule_work() can still queue the work.
>
> rawsock_destruct() does not cancel tx_work, and the queued work holds no
> socket reference. When the worker runs, rawsock_tx_work() would read
> freed memory:
>
> struct sock *sk = to_rawsock_sk(work);
> struct nfc_dev *dev = nfc_rawsock(sk)->dev;
> u32 target_idx = nfc_rawsock(sk)->target_idx;
> ...
> if (sk->sk_shutdown & SEND_SHUTDOWN) {
>
> Would it close this window to set SEND_SHUTDOWN under sk_write_queue.lock
> in rawsock_release(), before cancel_work_sync()? Then either the
> completion sees the flag, or its schedule_work() ran before release took
> the lock, and cancel_work_sync() will cancel or wait for that work.
>
> Separately, the unlocked read-modify-write of sk->sk_shutdown in
> rawsock_release() races with the plain read here. It also races with the
> plain store in rawsock_report_error():
>
> sk->sk_shutdown = SHUTDOWN_MASK;
>
> KCSAN would likely report this as a data race. The rest of the
> networking stack accesses sk_shutdown with READ_ONCE()/WRITE_ONCE().
>
> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/179378590950.74297.14503648624087894519%40gmail.com