buglwIP - A Lightweight TCP/IP stack - Bugs: bug #52937, raw_input() must tell...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #52937: raw_input() must tell ip4/6_input() whether this protocol has been received

Submitter:  hanhui <hanhui03>
Submitted:  Sat 20 Jan 2018 07:02:45 AM UTC
   
 
Category:  IPv4 Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Thu 25 Jan 2018 11:57:26 AM UTC, comment #18: 

Fixed as suggested, thanks for the input.

> What advantage do you see having the value be at the end of the
> number space rather than sequential like an enumeration?


Hmm, only that we could have made these defines available to raw pcb recv callbacks. But that doesn't make sense, and internally, an enum is good.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 23 Jan 2018 03:04:31 PM UTC, comment #17: 

Simon,

Patch looks correct to me, couple of minor things:


I'd add would be some comments for the RAW_INPUT defines:

#define RAW_INPUT_NONE           0    /* pbuf did not match any pcbs */
#define RAW_INPUT_EATEN          1    /* pbuf handed off and delivered to pcb */
#define RAW_INPUT_DELIVERED      0xFF /* pbuf only delivered to pcb (pbuf can still be referenced) */


> I'm not too happy with the "delivered" being 0xFF


I would fine with it being 2. What advantage do you see having the value be at the end of the number space rather than sequential like an enumeration?

From the raw.h, I can't quiet tell if raw_input is part of the callback API or something internal to LwIP.  If not, we could change the function signature and use an enumerated type (instead of #define).

Joel Cunningham <jcunningham>
Group Member
Tue 23 Jan 2018 11:38:53 AM UTC, comment #16: 

Attached is my proposed fix. I'm not too happy with the "delivered" being 0xFF, but at least this ensures the old constants "0" and "1" are kept.

If we do it that way, raw_input() docs need updating, of course.

(file #43033)

Simon Goldschmidt <goldsimon>
Group administrator
Mon 22 Jan 2018 05:48:41 PM UTC, comment #15: 

OK, well, this patch just describes the problem with the shortest code, and do not add a variables and keep the consistency of the received function parameters is good. I agree!

raw recv callback I'm not in favor of the changes, because it may lead some applications incompatible, Here leads to another proposal that many applications may have been made as a library, the macro definition in the header file should be consistent with the previous definition, which can improve compatibility

The above suggestions are for reference only ^_^

hanhui <hanhui03>
Mon 22 Jan 2018 05:43:27 PM UTC, comment #14: 


> This patch seems to fix unknown ip protocols only.


LWIP_HOOK_IP4_INPUT and LWIP_HOOK_IP6_INPUT have this issue in-addition to raw_input if the hook copies the pbuf rather than eating it.

Joel Cunningham <jcunningham>
Group Member
Mon 22 Jan 2018 04:40:24 PM UTC, comment #13: 

Let's talk about what we want to achieve, not how.
- 'eaten' means 'delivered', 'packet handled' and 'pbuf freed'
- 'delivered' means the ip protocol has been matched. This suppresses ICMP for unknown protocols but does not prevent pbufs for tcp, udp or igmp etc being delivered to the real protocol stack

This patch seems to fix unknown ip protocols only. There's nothing wrong with that. It doesn't mean any other change. We should have a separate discussion whether changes to the raw recv callback are required.

I'd still favour a return code over an additional argument though.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 22 Jan 2018 03:54:59 PM UTC, comment #12: 


> but 'eaten' and 'deliver' are two different meanings, so just add a 'deliver' variable is OK!


Actually eaten implies the pbuf was delivered, really the 3 states are: no matching pcb, eaten, and copied.  Eaten and copy implied the pbuf was delivered, with the difference being what happens to the pbuf.


Joel Cunningham <jcunningham>
Group Member
Mon 22 Jan 2018 03:42:35 PM UTC, comment #11: 

lwip can keep the feature of 'eaten', I think it's a good design, but 'eaten' and 'deliver' are two different meanings, so just add a 'deliver' variable is OK!

hanhui <hanhui03>
Mon 22 Jan 2018 03:28:43 PM UTC, comment #10: 

This patch is very very very simple, Just a few lines, Linux handling of this place is also very simple. No need to change too much.

hanhui <hanhui03>
Mon 22 Jan 2018 03:21:03 PM UTC, comment #9: 

I haven't found the time to look at the patch, ywt, but from reading the comments, that's what I would have done, Joel!

I'm not familiar with how this is done in linux. I only know the user space raw sockets, I don't think you can influence icmp behaviour from there.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 22 Jan 2018 03:16:00 PM UTC, comment #8: 

The issue that hanhui pointed out is that there are three states that can happen in the raw_input, but the return value only communicates the first two possibilities:

1. No matching pcb.
1. Matching pcb zero-copy 'eats' the pbuf, no further processing can happen on it.
2. Matching pcb copies the pbuf, further processing can be done on it.

The netconn layer (recv_raw) always copies the pbuf, so eaten would never be 1.  From what hanhui described, it sounds like Linux doesn't have the zero-copy 'eaten' case.

I'm wondering if instead this additional state should be communicated from the raw_input return value, i.e. return 0 for no matching pcb, return 1 for eaten, return 2 for copied/delivered.


Joel Cunningham <jcunningham>
Group Member
Mon 22 Jan 2018 04:05:29 AM UTC, comment #7: 

This is an implementation, of course, there are many ways to amend.

(file #43012)

hanhui <hanhui03>
Sun 21 Jan 2018 01:36:19 PM UTC, comment #6: 

Linux processing is correct, I submit a patch tomorrow :-)

hanhui <hanhui03>
Sun 21 Jan 2018 12:50:40 PM UTC, comment #5: 

So under linux, you should have the same problem, right? I'm not sure if we should solve this differently in lwip?

Simon Goldschmidt <goldsimon>
Group administrator
Sun 21 Jan 2018 12:17:00 PM UTC, comment #4: 

Linux implementation is similar, all ip input packet will first enter the raw input function, if raw input matched, then return "delivered", then ip input function according to the packet protocol number (for example: udp, tcp ...) calls the relevant protocol input function, if packet protocol is not found in the protocol table, the ip input function checks to see if the raw input returned "delivered", and if not, the ICMP_DEST_UNREACH packet will be send.

hanhui <hanhui03>
Sun 21 Jan 2018 10:31:05 AM UTC, comment #3: 

OK thanks for explaining. That's true of course. I wonder how this is handled in linux though...

Simon Goldschmidt <goldsimon>
Group administrator
Sun 21 Jan 2018 02:59:13 AM UTC, comment #2: 

I explain what I mean. If an application, such as OSFP, uses a RAW socket to receive a special IP protocol packet, the raw_input() function handles it properly, but the ip4/6_input() will jump to the unrecognized protocol processing branch and send an ICMP packet. The processing is not correct. The raw_input() function must be able to tell the ip4/6_input() function that this special protocol packet has been received, but where raw_input() can not use eaten = 1 as the return value, otherwise other raw sockets may be wrong, so raw_input() need to add a parameter such as u8_t * is_recved, if the user raw socket correctly received the packet, raw_input() set this parameter to 1, ip4/6_input() function in the unrecognized protocol branch then not send the ICMP unrecognized packet if this parameter is 1.

hanhui <hanhui03>
Sat 20 Jan 2018 07:27:46 PM UTC, comment #1: 

I don't think I understand. 'ip4_input' calls 'raw_input'. If a packet is handled, 'raw_intput' is meant to return '1'. In that case, the pbuf is supposed to be deallocated by a raw pcb and 'ip4_input' would not send any ICMP response.

What doesn't work for you here?

Simon Goldschmidt <goldsimon>
Group administrator
Sat 20 Jan 2018 07:02:45 AM UTC, original submission:  

raw_input() must tell ip4/6_input() whether this protocol packet has been received(matched), then ip4/6_input() must not report a ICMP unrecognized error message.

hanhui <hanhui03>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #43012:  raw_deliver.patch added by hanhui03 (5KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jcunningham (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by hanhui03 (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-01-25 goldsimon StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2018-01-23 goldsimon Attached File- Added 0001-ip4-ip6-prevent-protocol-unreachable-after-raw-pcb-m.patch, #43033
    2018-01-22 hanhui03 Attached File- Added raw_deliver.patch, #43012

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code