[RFC] nbd: is the add_socket queue freeze needed on the initial (pre-start) connect path?

To: Josef Bacik <[email protected]>
Cc: Christoph Hellwig <[email protected]>,
    Jens Axboe <[email protected]>,
    Ming Lei <[email protected]>,
    [email protected],
    [email protected]

Hi all,

This is a question / RFC, not a patch -- I'd like your opinion before
proposing anything, because the code in question is a deliberate,
stable-tagged UAF fix and I don't want to weaken it.

Background / motivation
-----------------------
We run large numbers of Firecracker microVMs whose block devices are
backed by NBD. On the VM-create hot path each microVM does an initial
NBD connect (netlink NBD_CMD_CONNECT), and we launch many of them
concurrently on the same many-core host. Profiling the create path
shows a large chunk of kernel time is spent in blk_mq_freeze_queue()
during the connect: each freeze waits for a full RCU grace period, which
on these many-core hosts we measure at roughly 35-50ms. Because several
connects run concurrently, these grace-period waits stack up and become
a visible tail-latency contributor on VM start.

One of the freezes on that path is in nbd_add_socket(), added by

  b98e762e3d71 ("nbd: freeze the queue while we're adding connections")

with the comment "We need to make sure we don't get any errant requests
while we're reallocating the ->socks array." I understand this freeze
fences the krealloc() of config->socks[] against a concurrent
nbd_queue_rq() -> nbd_handle_cmd(), which dereferences
config->socks[index] and config->num_connections -- i.e. it prevents a
use-after-free / out-of-bounds read, and it was Cc: stable. I am *not*
questioning that this is needed while the device is live.

Observation about the *initial* connect
----------------------------------------
On the initial connect, before the device is started (nbd->pid == 0),
nbd_add_socket() runs while:

  - capacity is still 0. set_capacity_and_notify() is only reached in
    nbd_set_size() after the "if (!nbd->pid) return 0;" early return,
    and on both the netlink and ioctl paths nbd_add_socket() runs
    strictly before nbd_start_device() sets nbd->pid; and

  - the queue advertises no write cache / discard / write-zeroes yet
    (those queue_limits features are also only set in the nbd->pid
    branch of nbd_set_size()).

Walking submit_bio_noacct() with capacity == 0 and no features, every
I/O op seems to be rejected before it can become a request that reaches
nbd_handle_cmd():

  - READ/WRITE with sectors: bio_check_eod() -> -EIO (maxsector == 0);
  - flush (REQ_PREFLUSH): !bdev_write_cache() path strips the flush and
    completes a zero-sector bio with BLK_STS_OK without dispatch;
  - DISCARD / WRITE_ZEROES / SECURE_ERASE / ZONE_*: not_supported since
    the corresponding limits are 0 / not set;
  - passthrough (DRV_IN/OUT): not_supported on the bio submit path, and
    nbd issues none itself.

The only theoretical gap I can see is a zero-length, non-flush data bio
(bio_check_eod() skips the check when nr_sectors == 0), but the VFS /
direct-IO layers don't actually submit zero-length data bios.

The question
------------
Given the above, is the nbd_add_socket() freeze effectively redundant on
the *initial*, pre-start (!nbd->pid) connect, where no I/O can reach the
driver? If so, would any of the following be acceptable, or is the
freeze intentionally kept unconditional for robustness?

  1) Skip the freeze only when (!nbd->pid && get_capacity(disk) == 0),
     falling back to the stock freeze otherwise (fail-shut);
  2) Some cheaper barrier than a full-queue-freeze RCU grace period for
     this "no I/O possible yet" case;
  3) Leave it as-is -- the "no I/O reaches the driver" property relies on
     a non-local invariant spanning bio_check_eod(), the flush filter,
     the pre-start queue_limits, and the set_capacity() ordering, and you
     consider that too fragile to build on.

My instinct is that (3) is a legitimate answer and that the invariant is
fragile, which is exactly why I'm asking rather than sending a patch. If
there's a direction you'd be willing to take, I'm happy to do the work
and the testing.

Thanks,
Zhichao Huang <[email protected]>