You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/01/31 04:10:35 UTC

[GitHub] [incubator-nuttx] WaterBirdWayOrigin opened a new pull request #188: Fixed USB RNDIS not to send over MTU packet

WaterBirdWayOrigin opened a new pull request #188: Fixed USB RNDIS not to send over MTU packet
URL: https://github.com/apache/incubator-nuttx/pull/188
 
 
   USB RNDIS sent the packet which has
    CONFIG_ETH_PKTSIZE + CONFIG_NET_GUARDSIZE.
   The maximum size of Ethernet packet should be CONFIG_ETH_PKTSIZE.
   
   Signed-off-by: Koichi Okamoto <Ko...@sony.com>

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-nuttx] patacongo edited a comment on issue #188: Fixed USB RNDIS not to send over MTU packet

Posted by GitBox <gi...@apache.org>.

patacongo edited a comment on issue #188: Fixed USB RNDIS not to send over MTU packet
URL: https://github.com/apache/incubator-nuttx/pull/188#issuecomment-580740973

There is a lot more that we would need to discuss to decide what to do with CONFIG_NET_GUARDSIZE. Currently it is a very small value, defaulting to only two bytes. So it is not a memory hog and the simplest response is to keep it present in all drivers for commonality.

It should (eventually) be larger and common for all drivers. We need to look at how it is used today and how it might be used tomorrow. There is a probably a lot more involved that you are considering. It is not just for hardware packet receipt:

For packet receipt, it is necessary for some hardware, but not others. Often the hardware will DMA 2 byte FCS at the end of the package, or other hardware-specific info. That is the driver-specific part that you are considering but that is only part of the whole story.

There are several issues for packet transmission. These are less well defined and need further study, but we need to keep all of the driver packet definitions in place until we understand how we are going to handle these things:

1. Memory overrun bug. There was in the past, a bug that caused write past the end of the buffer by a couple of bytes during TX message formatting. I don't know if that bug still exists, but the minimum CONFIG_NET_GUARDSIZE was sufficient to eliminate the bug. That is why it has the name GUARD: Its primary purpose is to protect from overrunning the packet buffer and corrupting the following memory.

I do no know if we have any such bugs today. Perhaps they do? Perhaps they do not? Having such a guard is a good thing for reliability in any case.

2. Variable size IP/TCP headers. There is a limitation in the way IP packets are formatted now. Basically they are formatted like this:

- When the packet is received a pointer to the location of the payload is set (d_appdata). This is an offset into the packet buffer For TCP, that accounts for the MAC/Ethernet header, the minimum IPv4/IPv6 header size, and the minimum TCP header size.
- The TCP payload data is written at that location using a constant MSS,
- The minimum sized IPv4/IPv6 headers and the minimum sizde TCP header are added below the payload, and finally
- The MAC/Ethernet header as add at the very beginning of the buffer.

The problem is, of course, is that IPv4/IPv6 and TCP headers are not constant. Using the minimum size for these headers precludes using IPv4/IPv6 or TCP header options. There are currenly only a few places where options are required and these haveto be handled as special cases. This needs to be formalized into the common packet formatting logic. Otherwise, the network will never be able to handle advanced features like tunneling or NAT.

I am thinking that this should be like:

- When the packet is received a pointer to the location of the payload is set (d_appdata). This is an offset into the packet buffer For TCP, that accounts for the MAC/Ethernet header, the **maximum** IPv4/IPv6 header size, and the **maximum** TCP header size.
- The TCP payload is written at that location,
- The **correctly** sized IPv4/IPv6 headers and the **correctly** sized TCP header are added below the payload, and finally
- The MAC/Ethernet header as added.
- The start offset of the packet in the packet is no longer zero, but some variable offset into the packet buffer.

The key to making this all work is:

- Keep CONFIG_NET_GUARDSIZE in all driver buffers, and
- Set the CONFIG_NET_GUARDSIZE to the maximum size of IPv4/IPv6 and TCP options (depending on which IP enabled and if TCP is enabled)

3. Variable MSS. Closely related to this is the MSS which is the maximum size of the payload. Currently that is a constant because it assumes the minimum header lengths. It should be variable.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [incubator-nuttx] patacongo commented on issue #188: Fixed USB RNDIS not to send over MTU packet

Posted by GitBox <gi...@apache.org>.

patacongo commented on issue #188: Fixed USB RNDIS not to send over MTU packet
URL: https://github.com/apache/incubator-nuttx/pull/188#issuecomment-580740973

I do no know if we have any such bugs today. Perhaps they do? Perhaps they do not? Having such a guard is a good thing for reliability in any case.

2. Variable size IP/TCP headers. There is a limitation in the way IP packets are formatted now. Basically they are formatted like this:

I am thinking that this should be like:

- When the packet is received a pointer to the location of the payload is set (d_appdata). This is an offset into the packet buffer For TCP, that accounts for the MAC/Ethernet header, the **### maximum** IPv4/IPv6 header size, and the **### maximum** TCP header size.
- The TCP payload is written at that location,
- The **### correctly** sized IPv4/IPv6 headers and the **### correctly** sized TCP header are added below the payload, and finally
- The MAC/Ethernet header as added.
- The start offset of the packet in the packet is no longer zero, but some variable offset into the packet buffer.

The key to making this all work is:

- Keep CONFIG_NET_GUARDSIZE in all driver buffers, and
- Set the CONFIG_NET_GUARDSIZE to the maximum size of IPv4/IPv6 and TCP options (depending on which IP enabled and if TCP is enabled)

3. Variable MSS. Closely related to this is the MSS which is the maximum size of the payload. Currently that is a constant because it assumes the minimum header lengths. It should be variable.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [incubator-nuttx] WaterBirdWayOrigin commented on issue #188: Fixed USB RNDIS not to send over MTU packet

Posted by GitBox <gi...@apache.org>.

WaterBirdWayOrigin commented on issue #188: Fixed USB RNDIS not to send over MTU packet
URL: https://github.com/apache/incubator-nuttx/pull/188#issuecomment-581245103

@patacongo

Thank you for your discussion with CONFIG_NET_GUARDSIZE.

Does each of drivers have the responsibility of the limitation of ethernet packet length?
If so, each of drivers may treat the appropriate ehternet packet length in it regardless of CONFIG_NET_GURARDSIZE.

It seems to me that CONFIG_NET_GUARDSIZE is the spirit of uIP. I am not sure but
if TCP/IP statemachine or checksum algorism doesn't need CONFIG_NET_GUARDSIZE,
this value can be removed from NuttX code.

Upon reconsidering the matter, driver level modification such as USB RNDIS can limit
an affected code area. When nobody uses CONFIG_NET_GUARDSIZE in driver, as well as TCP/IP statemachine and application, NuttX can remove this CONFIG safely.

I will also discuss the lack of error handling of rndis_fillrequest() return 0 (Ethernet length is 0 and over CONFIG_NET_ETH_PKTSIZE). In this case,there is no RNDIS BULKIN header. I choose EINVAL in this case. Is this reasonable modification?

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [incubator-nuttx] patacongo commented on issue #188: Fixed USB RNDIS not to send over MTU packet

Posted by GitBox <gi...@apache.org>.

patacongo commented on issue #188: Fixed USB RNDIS not to send over MTU packet
URL: https://github.com/apache/incubator-nuttx/pull/188#issuecomment-580767263
 
 
   I just put the above information into a Wiki page at:  https://cwiki.apache.org/confluence/display/NUTTX/CONFIG_NET_GUARDSIZE

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-nuttx] patacongo commented on issue #188: Fixed USB RNDIS not to send over MTU packet

Posted by GitBox <gi...@apache.org>.

patacongo commented on issue #188: Fixed USB RNDIS not to send over MTU packet
URL: https://github.com/apache/incubator-nuttx/pull/188#issuecomment-580601132
 
 
   No, that is the way that the packet size is configured for all network drivers.  We will not make an exception here. The GUARD_SIZE is configurable.  If you want it to be zero set it to zero in your configuration.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #188: Fixed USB RNDIS not to send over MTU packet

Posted by GitBox <gi...@apache.org>.

xiaoxiang781216 commented on issue #188: Fixed USB RNDIS not to send over MTU packet
URL: https://github.com/apache/incubator-nuttx/pull/188#issuecomment-580610226
 
 
   But GUARD_SIZE is global option, how to handle the case that both ethernet(require guard bytes) driver and rndis driver enable?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-nuttx] patacongo closed pull request #188: Fixed USB RNDIS not to send over MTU packet

Posted by GitBox <gi...@apache.org>.

patacongo closed pull request #188: Fixed USB RNDIS not to send over MTU packet
URL: https://github.com/apache/incubator-nuttx/pull/188
 
 
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services