Missing drop implementations in netdev_driver_t::recv

[maribu]

Description

Some implementations of netdev_driver_t::recv(netdev_t *dev, void *buf, size_t len, void *info) do not implement the drop case ((buf == NULL) && (len != 0)). This issue should track the effort to provide the missing implementations.

In case the upper layer used for the driver never uses the drop feature of recv, an implementation of the drop feature is not required, if instead corresponding assert()s are added and this is documented properly.

Tracking

Driver	State	Pull Request
ata8520e	Not affected (*)	-
at86rf2xx	Fixed, backport in progress	#10285
cc110x	Rewrite contains fix	#10340
cc2420	Fix merged	#10416
cc2538_rf	Not affected
enc28j60	Fix merged	#9806
encx24j600	Fix merged	#10415
esp32/esp-eth	Not affected	-
esp32/esp-now	Not affected	-
esp32/esp-wifi	Not affected	-
esp8266	Not affected	-
ethos	Not affected	-
kw2xrf	Fixed	4ebcd7c
kw41zrf (#7107)	Not affected	-
mrf24j40	Fixed, PR merged	#9562
netdev_tun	Not affected
nrfble	asserts present, doc requried
nrfmin	Not affected	-
rn2xx3	Not affected (*)	-
slipdev	Not affected
sx127x	AFFECTED
socket_zep	Fixed	fa2d9bd
w5100	Fix merged	#10412
xbee	Not affected	-

(*) Driver does not use the netdev_driver_t API, so it cannot be affected.

---

Update 1: Fixed typo
Update 2: Updated state of mrf24j40
Update 3: Added kw41zrf
Update 4: Cleaned up legend
Update 5: Added link to PR for w5100 fix
Update 6: Added link to PR for encx24j600, updated state of encx24j600 & w5100
Update 7: Added link to PR for cc2420 and updated its state
Update 8: Added esp32 netdev_driver_t implementations
Update 9: Added esp8266, confirmed that ata8520e and rn2xx3 are not using the netdev_driver_t API (yet) and are, thus, unaffected
Update 10: Corrected state for at86rf2xx
Update 11: enc28j60 and cc2420 are now fixed
Update 12: w5100 is now fixed
Update 13: encx24j600 fix merged
Update 14: Added cc2538_rf and netdev_tun
Update 15: After looking more closely into cc2538_rf, the error handling in the drop case with pkt_len > CC2538_RF_MAX_DATA_LEN is just fine. So cc2538_rf is not affected

[maribu]

@aabadie: Can you confirm that the ata8520e is not affected?
@bergzand : Can you confirm that the mrf24j40 is not affected?
@aabadie: Can you confirm that the rn2xx3 is not affected?

Thanks :-)

[bergzand]

@maribu Thanks for picking this up.

@bergzand : Can you confirm that the mrf24j40 is not affected?

Should have been fixed with #9562

[maribu]

Wow, that was a fast response time :-) Thanks, I updated the table accordingly

[jnohlgard]

@maribu thanks for taking this on. I checked the proposed kw41zrf driver in #7107 and it does implement drop and size correctly, dropping the frame when buf == NULL, len != 0

A related API question, should the buffered frame be dropped when recv is called with buf != NULL and len != 0, but len < frame_len?
The alternative to dropping would be to keep the buffered frame until the caller provides a big enough buffer, or explicitly drops it using buf=NULL.

[bergzand]

A related API question, should the buffered frame be dropped when recv is called with buf != NULL and len != 0, but len < frame_len?
The alternative to dropping would be to keep the buffered frame until the caller provides a big enough buffer, or explicitly drops it using buf=NULL.

Good question. I have a few ideas that could benefit from being able to first read part of the frame before reading the full frame. For example a bit of netdev that first reads the link layer addresses to check whether the device wants the frame or that it should be dropped. No need to spend time pulling in the full frame from the radio then.

[bergzand]

@maribu There is also #10285 which claims to address this issue for the at86rf2xx

[maribu]

The alternative to dropping would be to keep the buffered frame until the caller provides a big enough buffer, or explicitly drops it using buf=NULL.

This would allow the most flexibility, but upper layer code must be prepared to explicitly drop if it does not intend to retry with a bigger buffer. But as drivers can often share upper layer code, I would argue to let the upper layer handle this. Moving complexity to more common used code seems to be a better choice regarding maintainability.

Also the documentation on this could be improved quite a bit:

• The precondition conflicts with the size and drop feature of the recv()
• This would be a good place to state whether the frame should be dropped in case of a too small buffer
• This would be a good place to state whether the part of the frame fitting in the buffer should be written there in case of a too small buffer

[maribu]

@bergzand

There is also #10285 which claims to address this issue for the at86rf2xx

I think this is a similar issue, but the drop feature seems to be not missing in the current master:

RIOT/drivers/at86rf2xx/at86rf2xx_netdev.c

Lines 152 to 157 in 836fe3d

	/* drop packet, continue receiving */
	if (len > 0) {
	/* set device back in operation state which was used before last transmission.
	* e.g RX_AACK_ON */
	at86rf2xx_set_state(dev, dev->idle_state);
	}

Update: This is a backport of the fix of this issue. So in current master it is fixed. I will add the PR to the tracking

[maribu]

@gebart: Thanks for raising the question on how recv() should behave when the provided buffer is too small. E.g. w5100 and cc2420 handle this differently than the API documentation states: The silently truncate the incoming frame and do not return any error to the upper layer, but the API states that -ENOBUFS has to be return. I think silently truncating is not a good idea, as the upper layers will have no chance to detect this issue. Let me open another issue for that to first discuss how that case should be handled correctly and then to track the conversion.

[maribu]

@gebart: I created #10413 for that

[maribu]

@PeterKietzmann, @jfischer-phytec-iot, @gebart: Regarding the kw2xrf: After taking a closer look, I think it is not affected. The driver does not seem to manually drop the incoming frame in any case (even when retrieving the frame successfully), so I guess just not retrieving the frame implicitly drops it. Is this correct?

[jnohlgard]

If the kw2xdrf radio works like the kw41zrf radio, then the buffer may be overwritten whenever the transceiver is switched back into R sequence, which could happen after reading the frame length depending on the driver implementation. So there may be a race condition bug waiting to happen where reading the size removes the frame buffer write protection, and a new frame is received before the actual read out of the frame buffer has been done in the second call to recv.
kw41zrf solves this by switching to standby after a completed RX until the frame is read out or dropped, but there is also a PB_PROTECT bit which can be used to write protect the RX buffer which I did not know about until recently. I have not explored this protection bit in #7107 however.
Edit: Fixed some factual mistakes in the description of the receive procedure

[bergzand]

@maribu There are some ESP{32,8266} module that also implement netdev (@gschorcht ping :) ), not sure what the status there is though.

[maribu]

@aabadie: You seem to have worked a lot on the sx127x driver. To me it looks like it is not affected by the bug, as the buffer is implicitly dropped. Is this correct?

[maribu]

@maribu There are some ESP{32,8266} module that also implement netdev (@gschorcht ping :) ), not sure what the status there is though.

The ESP32 is not affected (checked for esp-eth, esp-now and esp-wifi). I didn't checked on the ESP8266 yet.

[gschorcht]

@bergzand ESP8266 and ESP32 netdev drivers drop packets according to interface defitinition. When I started to implement them, I read the API documentation for netdev_driver::recv very carefully 😉

[bergzand]

@bergzand ESP8266 and ESP32 netdev drivers drop packets according to interface defitinition. When I started to implement them, I read the API documentation for netdev_driver::recv very carefully wink

Awesome, good to hear that. :)

[jia200x]

I guess this should stay open? @miri64

[miri64]

Yepp. This was an automatic close due to #10412 being merged, which contained ea3edef with the magic word ;-).

[maribu]

@PeterKietzmann, @jfischer-phytec-iot, @gebart: Regarding the kw2xrf: After taking a closer look, I think it is not affected. The driver does not seem to manually drop the incoming frame in any case (even when retrieving the frame successfully), so I guess just not retrieving the frame implicitly drops it. Is this correct?

@PeterKietzmann, @jfischer-phytec-iot, @gebart: Could one of you confirm it is unaffected?

[miri64]

@maribu can this be left close now?

[kaspar030]

kaspar030 closed this in [727b4ca](/RIOT-OS/RIOT/commit/727b4cac1c850ccd06d825e3c50dfa18a81c33c7) 4 minutes ago

Ups, this was a too-eager autoclose. I'll just update the list.

[miri64]

@maribu, while I was working on the tracking list for #11163: I think cc2538_rf and netdev_tap are missing in your list. Is there a reason for that

[maribu]

No. I just got confused with some network drivers being in drivers/ and others in cpu/ :-(

[maribu]

Both are not affected

[miri64]

No. I just got confused with some network drivers being in drivers/ and others in cpu/ :-(

The distinction is simple: in drivers are devices that can be on a board, the one in the one in cpu are specific to that MCU.

[maribu]

Oh, I totally forgot about this issue :-( Did someone by chance fix the remaining drivers?

[maribu]

The kw2xrf driver seems not effect: To me it seems that no command to flash the FIFO is ever triggered during RX - so apparently the device will just starting to write to the head of the FIFO again when receiving the next frame and no flushing is needed. Thus, the driver is not effected.

However, the driver goes back to RX the very moment the frame is completely received, regardless on whether the netdev_driver_t::recv() is ever called. I guess it would be possible that when netdev_driver_t::recv() is not fetching the frame soon enough, part of the buffer could already be overwritten with the next frame. That is certainly not ideal either. Maybe @smlng can confirm / correct my observations from a brief look at the code?

[miri64]

Maybe @smlng can confirm / correct my observations from a brief look at the code?

@PeterKietzmann @MrKevinWeiss or @leandrolanzieri can you do that, please. Also, what the state of the other drivers marked as AFFECTED in OP?