net_buf_append_bytes() usage may cause a failed assertion

[kderda]

Describe the bug
Depending on the number of bytes provided as net_buf_append_bytes()'s it can allocate additional buffers under the hood. However, when the buffer pool is of type 'fixed' the default allocation method within this function calls net_buf_alloc_len() which tries to allocate a given number of bytes and it contains an assertion which checks whether the allocated size is bigger or equal to requested one.

#if __ASSERT_ON
		size_t req_size = size;
#endif
		timeout = sys_timepoint_timeout(end);
		buf->__buf = data_alloc(buf, &size, timeout);
		if (!buf->__buf) {
			NET_BUF_ERR("%s():%d: Failed to allocate data",
				    func, line);
			net_buf_destroy(buf);
			return NULL;
		}

#if __ASSERT_ON
		NET_BUF_ASSERT(req_size <= size);
#endif

For fixed buffer pools this assertion can be hit quite easily if the number of bytes provided to net_buf_append_bytes() fills more than 2 fixed buffers.

To Reproduce
The following snippet presents the test case:

NET_BUF_POOL_FIXED_DEFINE(fixed_pool, 4, 64, 0, NULL);

int test(void) {
        struct net_buf *buf;
        uint8_t data[256];

        buf = net_buf_alloc(&fixed_pool, K_NO_WAIT);
        net_buf_append_bytes(buf, sizeof(data), data, K_NO_WAIT, NULL, NULL);
}

Expected behavior
net_buf_append_bytes() should allocate multiple struct net_bufs without assertion.

Impact
Current implementation may cause unexpected faults, e.g. while processing data from the network.

Additional context
This issue has a workaround - user can provide allocate_cb argument while calling net_buf_append_bytes() and provide a custom allocator but one will probably find out after debugging this issue.

[github-actions]

Hi @konradderda! We appreciate you submitting your first issue for our open-source project. 🌟

Even though I'm a bot, I can assure you that the whole community is genuinely grateful for your time and effort. 🤖💙

[jukkar]

@konradderda good catch! Would you be able to submit a fix for this?

[kderda]

It was my idea at first but it's a bit tricky (if my understanding is correct):
The default allocation goes by:

                        /* Allocate from the original pool if no callback has
			 * been provided.
			 */
			pool = net_buf_pool_get(buf->pool_id);
			frag = net_buf_alloc_len(pool, len, timeout);

The net_buf_alloc_len() contains the assertion which is triggered when data_alloc() inside it returns less raw bytes than requested. At the moment, there is no way to determine that the given pool's type.

The possible solutions I am thinking off:

1. Change the public function net_buf_alloc_len() to provide a chain of buffers when needed (but it feels a little wrong)
2. Provide some kind of additional information - either like is_fixe' flag or an additional pointer to specific buffer allocator which could be called directly or with a wrapper instead of net_buf_alloc_len() in the code snippet above

[jhedberg]

@konradderda @jukkar I took a quick look at the related code. Perhaps struct net_buf_data_alloc needs another member to indicate the maximum size that can be allocated at one time, which would then be used by net_buf_append_bytes() to make decisions on how to fragment the buffers. I suppose that would also mean removing the data_size member from struct net_buf_pool_fixed, since it'd become redundant. Anyway, this is just my first thoughts after spending a short time analysing the issue.

[kderda]

@jhedberg @jukkar I've opened the PR: #68413