Re: [PATCH net-next v2 6/7] net: mctp: usblib: Add initial kunit tx tests

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

> [Severity: Low]
> 
> Is completing the transfer inline here consistent with the documented
> ops->send contract?
> 
> include/linux/usb/mctp-usb.h says, for struct
> mctp_usblib_tx_ops::send:
> 
> 	/* Start a USB TX for @data. On returning success, the
> implementation
> 	 * must arrange for mctp_usblib_tx_send_complete() to be
> called at some
> 	 * later point (eg., on urb completion).
> 	 */
> 
> mctp_usblib_test_tx_send() instead calls
> mctp_usblib_tx_send_complete()
> (which runs mctp_usblib_tx_stats_update() and
> mctp_usblib_tx_ctx_free())
> and then returns 0, so the tx context is already freed at the point
> success is reported.  The only in-tree implementer,
> mctp_usb_out_complete() in drivers/net/mctp/mctp-usb.c, completes
> asynchronously from URB completion.
> 
> Nothing breaks today: mctp_usblib_tx_send() reads ctx->tx->ops.send,
> buf
> and ctx->len before the call and only returns the result, and both
> send
> sites in mctp_usblib_tx_push() touch the context afterwards only
> under
> if (rc).  But if the library ever legitimately inspects the context
> after
> a successful ->send(), this harness would report it as a KASAN
> use-after-free in the test rather than a driver bug.
> 
> Would it be better to either record the context and complete it after
> the
> push returns, or to relax the header comment to explicitly allow
> synchronous completion?
> 
There is no "instead". The documented function is called. Inspecting
the context after a send would not be legitimate. This appears to be a
false positive.


> [Severity: Low]
> 
> Do the assertions in this test actually observe either of the two
> properties named in the comment?
Seemingly.

> [Severity: High]
> 
> Does this static_assert() compile?
Yes.