canbus: isotp: Violations of k_fifo and net_buf API usage

[jhedberg]

subsys/canbus/isotp/isotp.c has several issues in the way it's using k_fifo and net_buf APIs, which are currently blocking the progress of #36370

I'll first outline the k_fifo & net_buf API assumptions that I believe are being violated:

k_fifo

It is expected that the space of the size of a pointer (4 or 8 bytes) is reserved in beginning of elements stored inside a k_fifo. This is currently a singly linked list, but how exactly this is implemented is a k_fifo internal implementation detail. Or to put it in other words, users of the k_fifo API should not touch (read or write) this reserved memory as long as the element resides inside a k_fifo.

net_buf

Beginning of the net_buf struct definition (which is the basis of a lot of the argumentation that follows). The memory covered by the union below is private to k_fifo while elements are inside it:

struct net_buf {
	union {
		/** Allow placing the buffer into sys_slist_t */
		sys_snode_t node;

		/** Fragments associated with this buffer. */
		struct net_buf *frags;
	};

To preserve compactness of struct net_buf (i.e. not to require another struct member outside of the union) the fragment list of buffers is encoded as consecutive k_fifo elements while the buffer is inside a k_fifo. The k_fifo internal details are not violated here thanks to the usage of k_fifo_put_slist(). This means that net_buf_get() and net_buf_put() APIs must be used when getting buffers from a k_fifo or putting buffers into a k_fifo. Buffers are encoded as being fragments to a "parent" buffer with the help of a dedicated flag bit in buf->flags. This flag bit will be set if the buffer has follow-up fragments in the FIFO. The details of how this encoding is accomplished is also a net_buf internal detail, and there are in fact plans/ideas to make buf->frags an independent struct member which would remain untouched while the buffer gets passed through a FIFO. As long as API users stick to net_buf_get() and net_buf_put() APIs such a transition can be made without any breakage, but issues arise as soon as users start accessing the internals of how this is implemented.

Another expectation of net_buf API usage, is that the user always owns a reference to the buffer that it's handling.

Violations in isotp.c

isotp.c violates all of the above API principles. The problematic code is mainly in the isotp_recv() and pull_frags() functions.

isotp_recv() issues:

1. isotp_recv() will call k_fifo_peek_head() on a FIFO containing net_bufs.
2. After the above step the code doesn't use net_buf_ref() to get a reference
3. Through a subsequent call to pull_frags() the code ends up accessing buf->frags which is an alias for the reserved memory of k_fifo elements, i.e. the k_fifo API is also being violated here.

pull_frags() issues:

1. pull_frags() is already in trouble since it has gotten a buffer that's inside a FIFO and owns no reference to it, meaning that it has neither valid read or write access to it.
2. The code goes on to iterating the buf->frags list (k_fifo private memory at this point) and making strange "blind" calls to k_fifo_get() which ignore the return value
3. What's odd with this code is that it does not clear the fragment flag in buf->flags (an internal net_buf detail) and doesn't therefore seem to be able to distinguish between multiple buffers with their own independent fragment lists. Not clearing the flag will also cause undefined behaviour, since net_buf_put() assumes the flag is always zero prior to being given a buffer (i.e. the flag will be set but never cleared).
4. The usage of net_buf_ref() and net_buf_unref() in pull_frags() looks quite odd as well (too much so for me to determine if it's correct or not).

Proposal

The code is already using net_buf_put() to insert buffers (and their fragment lists) into the FIFO, but it needs to be rewritten to use net_buf_get() when getting buffers from the FIFO. Most likely there should be a new ctx->buf pointer in struct isotp_recv_ctx which is used to keep a buffer fetched with net_buf_get() as long as it has some data in it, and keep getting data from that buffer as long as the pointer is non-NULL (and after that go back to fetching another buffer with net_buf_get().

The net_buf API offers some fragment-related convenience functions that may be useful for dealing with the new ctx->buf such as calling ctx->buf = net_buf_frag_del(NULL, ctx->buf) when all data has been consumed from the current ctx->buf.

[martinjaeger]

Thanks @jhedberg for your detailed explanations regarding this bug.

Your proposal makes sense and I have implemented it accordingly in #40162.