OpenFastPath / ofp

OpenFastPath project
BSD 3-Clause "New" or "Revised" License
349 stars 126 forks source link

No support for segmented odp packet in tcp_output & ofp_sosend_dgram #284

Open HsuJv opened 1 year ago

HsuJv commented 1 year ago

Hi there

I know that we have set the Interface MTU to 1500 at https://github.com/OpenFastPath/ofp/blob/f6580bb675386b0599390daaabbb0a06c1c1be0c/include/api/ofp_config.h#L61 which is smaller than the packet size(1856) set at https://github.com/OpenFastPath/ofp/blob/f6580bb675386b0599390daaabbb0a06c1c1be0c/include/api/ofp_config.h#L56

But in my use case, I'd like to make the MTU configurable to meet the requirements of jumbo frames (both TCP & UDP).

If I increase my MTU to some value that is greater than the packet size, it will result in a segmented odp_packet_t. But the memcpy here in ofp_tcp_output.c https://github.com/OpenFastPath/ofp/blob/f6580bb675386b0599390daaabbb0a06c1c1be0c/src/ofp_tcp_output.c#L870-L871 and the memcpy here in ofp_uipc_socket.c https://github.com/OpenFastPath/ofp/blob/f6580bb675386b0599390daaabbb0a06c1c1be0c/src/ofp_uipc_socket.c#L1009 do not check the segmented packets at all, which will cause a buffer overflow.

A simple approach to duplicate this issue is to reduce the SHM_PKT_POOL_BUFFER_SIZE to 1024 or smaller. Or calling ofp_sendto on the UDP sockets with len set to 2000 or greater.

Are there any plans to support this feature?

BRs.

bogdanPricope commented 1 year ago

Hi @HsuJv,

To support jumbo frames you will need to:

  1. re-write ofp_mtu_set() to actually set the the MTU on the interface (odp_pktio_maxlen_set() ??) considering max capabilities (capa.maxlen.max_output, capa.maxlen.max_input) etc. Can be tricky .. if I remember correctly, ODP is considering ETH header in this frame size.
  2. Set a pkt_pool.buffer_size large enough for your use-case
  3. Fix the two memory copies... Good find!!

My code is different .. cannot say right now if this is enough. I don't have plans to contribute to the project (I have my own version) but maybe the other members will. Else, it does not seems too complicated.

BRs

bogdanPricope commented 1 year ago

Btw, pkt segments are not well supported in the entire stack... you should try to keep it simple with large enough packets and first segment size = pkt size...

HsuJv commented 1 year ago

Hi @bogdanPricope

Thanks for the quick reply.

For your answer:

  1. Yes, I've already hooked the ofp_mtu_set with a function pointer to export it to my application so that I can change it dynamic ly.
  2. Actually it is not a feasible approach to increase buffer_size as it will consume a lot more memory for connections that run small packets and will decrease the capability of traffic offload of my application sharply.
  3. Yes the memory copies are the root cause. I've not walked through the whole packet paths but I found that the ofp_cksum in ofp_in_cksum.c did do the segment check. So I assume that at least some of the developer has taken the segmented packets into consideration.

So in a short, fixing the issues with segmented packets seems necessary for memory saving and may not be very complicated work. It seems that this project has been active again recently so I submitted issues here to let the contributors know.

BRs.