Re: [PATCH net-next v2 7/7] net: mctp: usblib: Add test for failing append

[James Lee <james@codeconstruct.com.au>]

> [Severity: Low]
> This isn't a bug, but len = 200 and the loop count of 3 below
> silently
> encode a dependency on MCTP_USB_1_0_XFER_SIZE (512) and MCTP_USB_BTU,
> without referencing either constant.
> 
> I traced the intended path and it does run today with span=false:
> each skb
> becomes 204 bytes after mctp_usblib_tx_skb_prepare() pushes the 4-
> byte
> header, push 1 creates the 512-byte ctx, push 2 appends (len 408,
> avail
> 104), and push 3 hits mctp_usblib_tx_append():
> 
> 	if (mctp_usblib_tx_avail(ctx) < skb->len)
> 		return -ENOBUFS;
> 
> with avail 104 < skb->len 204.
> 
> Could len and the iteration count be derived from
> MCTP_USB_1_0_XFER_SIZE / MCTP_USB_BTU instead, so the case keeps
> hitting
> the append-failure branch if those change?
> 
> Also, would tx_overflow be better named after the append failure the
> commit subject describes?
> 
Valid catch, will rename and remove the magic constants.

> 
> [Severity: High]
> Can this sleep with preemption disabled?  get_cpu_ptr() expands to
> preempt_disable() plus this_cpu_ptr(), so the whole loop runs in
> atomic
> context, and mctp_usblib_test_init_skb() allocates with GFP_KERNEL:
> 
> drivers/net/mctp/mctp-usblib-test.c:mctp_usblib_test_init_skb() {
> 	skb = __netdev_alloc_skb(ndev, length, GFP_KERNEL);
> 	KUNIT_ASSERT_NOT_NULL(test, skb);
> 	...
> }
> 
> GFP_KERNEL sets __GFP_DIRECT_RECLAIM, so __netdev_alloc_skb() takes
> the
> blocking slab path:
> 
> net/core/skbuff.c:__netdev_alloc_skb() {
> 	if (len <= SKB_WITH_OVERHEAD(SKB_SMALL_HEAD_CACHE_SIZE) ||
> 	    len > SKB_WITH_OVERHEAD(PAGE_SIZE) ||
> 	    (gfp_mask & (__GFP_DIRECT_RECLAIM | GFP_DMA))) {
> 		skb = __alloc_skb(len, gfp_mask, SKB_ALLOC_RX,
> NUMA_NO_NODE);
> 	...
> }
> 
> With CONFIG_DEBUG_ATOMIC_SLEEP, wouldn't the might_sleep() in the
> allocator produce a "sleeping function called from invalid context"
> splat
> every time this case runs?
> 
> Two further consequences of the same region:
> 
> The KUNIT_ASSERT_NOT_NULL() in mctp_usblib_test_init_skb() and the
> KUnit
> failure reporting behind KUNIT_EXPECT_EQ() also allocate with
> GFP_KERNEL,
> and the assert variant does not return:
> 
> lib/kunit/try-catch.c:kunit_try_catch_throw() {
> 	try_catch->try_result = -EFAULT;
> 	kthread_exit(0);
> }
> 
> so put_cpu_ptr() below is skipped and the thread exits still in
> atomic
> context, which kernel/exit.c reports:
> 
> kernel/exit.c:do_exit() {
> 	if (unlikely(in_atomic())) {
> 		pr_info("note: %s[%d] exited with preempt_count
> %d\n",
> 			current->comm, task_pid_nr(current),
> 			preempt_count());
> 		preempt_count_set(PREEMPT_ENABLED);
> 	}
> 	...
> }
> 
> mctp_usblib_tx_push() also takes a spinlock_t here:
> 
> drivers/net/mctp/mctp-usblib.c:mctp_usblib_tx_push() {
> 	spin_lock_irqsave(&tx->lock, flags);
> 	...
> }
> 
> On PREEMPT_RT that is a sleeping lock, so is acquiring it from an
> explicitly preempt-disabled section valid?
> 
> Note the driver itself uses GFP_ATOMIC on this path
> (mctp_usblib_tx_ctx_create() uses kzalloc_flex(..., GFP_ATOMIC)), so
> only
> the test's own allocation is out of context.
> 
> Since mctp_usblib_tx_stats_update() picks the counter with
> get_cpu_ptr(dev->dstats), could the skb allocation and the assertions
> move
> outside the pinned region, leaving only the counter read inside
> get_cpu_ptr()/put_cpu_ptr(), or could tx_drops be summed over
> for_each_possible_cpu() instead?
Good catch, will correct.

> [Severity: Low]
> This is the same finding as the len = 200 comment above: this
> aggregate
> drop count is the only oracle in the case.
> 
> The append-failure path yields 2 drops from the detached two-skb
> context
> plus 1 from the single-skb context, but so do other paths: if the ctx
> buffer size grew, if the MCTP_USB_BTU margin in
> mctp_usblib_tx_should_send() changed so the ctx flushed after push 2,
> or
> if all three skbs fit one ctx, the append branch never runs and stats
> is
> still 3.
> 
> Neither the number of ops.send() invocations, nor tx->cur_ctx, nor
> ctx->tx_xfers is checked here, while the neighbouring
> mctp_usblib_test_tx_failing_send() does:
> 
> 	KUNIT_EXPECT_EQ(test, rc, 0);
> 	KUNIT_EXPECT_NULL(test, tx->cur_ctx);
> 	KUNIT_EXPECT_TRUE(test, list_empty(&ctx->tx_xfers));
> 
> Would checking tx_drops == 0 after the first two pushes and counting
> the
> send attempts (2 sends carrying 2 and 1 packets) pin down the branch
> under
> test?
Fair point, will begin checking for expected behaviour between sends as
well.