net/devif devif_iob_poll: release iob only if data was consumed

[jnippula]

Summary

Currently in devif_iob_poll releases the IOB buffer unconditionally, even the devif_poll_connections() returned "false", indicating that no data has been handled. If IOB is unconditionally released, it may cause race condition in TCP accept connection (SYN packet) where tcp_alloc waits for free conn with net_breaklock called. During that period another core can trigger devif_poll and even the IOB is not yet connected to any connection, it is release and dev->d_iob is set to NULL. So, the dev->d_iob is released before tcp_alloc_accept has read the ip address info from the buffer. => kernel crash due to referencing NULL pointer.

Fixed to release IOB only in case it is really handled (bstop == true).

Testing

Verified with custom NXP i.mx9 device connected to laptop via ethernet cable. Running a tcp server in nuttx and connecting that from tcp client running of laptop.

[zhhyu7]

@jnippula Could you please try this patch to see if it can solve your problem? #17306
It might be root cause.

[jnippula]

@jnippula Could you please try this patch to see if it can solve your problem? #17306
It might be root cause.

Yes, that patch helps to the crash issue I had, because the tcp_alloc uses NET_BUFPOOL_TRYALLOC(), which sets timeout to 0. But there is still potential race condition when using NET_BUFPOOL_TIMEDALLOC or NET_BUFPOOL_ALLOC, where timeout > 0. The devif_poll should not release reserved IOB, which it does not consume.

[zhhyu7]

@jnippula Could you please try this patch to see if it can solve your problem? #17306
It might be root cause.

Yes, that patch helps to the crash issue I had, because the tcp_alloc uses NET_BUFPOOL_TRYALLOC(), which sets timeout to 0. But there is still potential race condition when using NET_BUFPOOL_TIMEDALLOC or NET_BUFPOOL_ALLOC, where timeout > 0. The devif_poll should not release reserved IOB, which it does not consume.

The protocol stack should not break net_lock in the TX or RX process, so the d_iob operation in an independent TX or RX process is safe. Even if we do not release d_iob in devif_poll, when we return to tcp_alloc_accept, the content of d_iob has already changed, and at this time, some of the values assigned to accept conn should be incorrect. Moreover, for the old network card drivers that do not use d_iob for packet transmission, not clearing d_iob after the interface returns will cause exceptions in subsequent processes. As far as I know, it should be like this, but it's also possible that there are some details I haven't noticed, This information is for your reference.

[jnippula]

Even if we do not release d_iob in devif_poll, when we return to tcp_alloc_accept, the content of d_iob has already changed, and at this time, some of the values assigned to accept conn should be incorrect.

Correct me if I'm wrong, but in my understanding in this case the content of the d_iob is not changed after devif_poll, because the connection being accepted is not yet in the g_active_tcp_connections pool, so the devif_poll can't find anything that would match the condition "if (dev == conn->dev)" (because tcp_alloc_accept fills that info after the tcp_alloc has returned and netlock is already restored).
So, I don't see any benefit or good reason why the drv->d_iob should be released in the devif_poll, if it does not find anything to do with that iob, as it is not linked to any connection.
If the buffer is consumed by the devif_poll (according to bstop) then the iob content is changed and shall also be released.

Moreover, for the old network card drivers that do not use d_iob for packet transmission, not clearing d_iob after the interface returns will cause exceptions in subsequent processes

The iob releasing of the flat buffer case is not touched. The flat buffer case is different, because the iob is only temporarily used and the content is explicitly copied from iob to flat buffer. The data is safe and accessible after the devif_poll.

If the devif_poll would periodically unconditionally just clean up d_iob from all drivers even there is nothing to be done (yet), I see a risk that some later implementation utilizing timed wait would face this kind of race condition again.

[zhhyu7]

Correct me if I'm wrong, but in my understanding in this case the content of the d_iob is not changed after devif_poll, because the connection being accepted is not yet in the g_active_tcp_connections pool, so the devif_poll can't find anything that would match the condition "if (dev == conn->dev)" (because tcp_alloc_accept fills that info after the tcp_alloc has returned and netlock is already restored). So, I don't see any benefit or good reason why the drv->d_iob should be released in the devif_poll, if it does not find anything to do with that iob, as it is not linked to any connection. If the buffer is consumed by the devif_poll (according to bstop) then the iob content is changed and shall also be released.

devif_poll is a sending behavior initiated by other conn bound to this dev. During this sending process, the TCP_SYN packet received this time and saved in d_iob for initializing accpet conn may be overwritten.

The iob releasing of the flat buffer case is not touched. The flat buffer case is different, because the iob is only temporarily used and the content is explicitly copied from iob to flat buffer. The data is safe and accessible after the devif_poll.

If the devif_poll would periodically unconditionally just clean up d_iob from all drivers even there is nothing to be done (yet), I see a risk that some later implementation utilizing timed wait would face this kind of race condition again.

If we do not set d_iob to NULL before devif_poll returns, when the driver that passes data into the protocol stack through d_buf will trigger the dev->d_iob != NULL condition in the next call ipv4_input(and other xxx_input), the previous d_iob will be directly input into the protocol stack as a new packet. Obviously, this is unreasonable.

[jnippula]

I understood now, the check "if (dev == conn->dev)" can of course be successful in case there are more than one TCP connection. There is no check which connection the data in current iob is related to. So if there is another active TCP connection, the devif_poll would use the device's current iob for sending for that one. I agree, checking whether devif_poll did something or not does not help, but the iob may be used and corrupted due to another connection.

So, this shall not be merged.