tasklwIP - A Lightweight TCP/IP stack - Tasks: task #14247, socket API: implement recvmsg...

 
 

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

task #14247: socket API: implement recvmsg msg_control support

Submitter:  Simon Goldschmidt <goldsimon>
Submitted:  Wed 23 Nov 2016 12:01:25 PM UTC
   
 
Category:  None Should Start On:  Wed 23 Nov 2016 12:00:00 AM UTC
Should be Finished on:  Wed 23 Nov 2016 12:00:00 AM UTC Priority:  5 - Normal
Status:  None Privacy:  Public
Assigned to:  None Percent Complete:  90%
Open/Closed:  Open Planned Release:  None
Effort:  0.00

Jump to the original submission

Mon 02 Apr 2018 02:08:04 PM UTC, comment #37: 


> Sorry if I'm being dense...so, this one?


Yes

Joel Cunningham <jcunningham>
Group Member
Sat 31 Mar 2018 08:02:06 PM UTC, comment #36: 

Sorry if I'm being dense...so, this one?

https://git.savannah.nongnu.org/git/lwip.git

Michael Zimmers <mzimmers>
Sat 31 Mar 2018 03:55:53 PM UTC, comment #35: 

Michael,

No, the version from git master is the current development, which there is no release for. The 2.0.3 release is a branch from master before any of the features you needed were added.

Joel Cunningham <jcunningham>
Group Member
Sat 31 Mar 2018 03:01:12 PM UTC, comment #34: 

Hi Joel -

So, this version, then?

http://download.savannah.nongnu.org/releases/lwip/lwip-2.0.3.zip

Thanks.

Michael Zimmers <mzimmers>
Sat 31 Mar 2018 02:39:05 PM UTC, comment #33: 

Michael,

If you only need recvmsg and IP_PKTINFO (the IPv4 implementation), that is already complete in the current development sources (on git master).  You'll need to enable LWIP_NETBUF_RECVINFO for IP_PKTINFo to work

Joel Cunningham <jcunningham>
Group Member
Sat 31 Mar 2018 01:58:14 PM UTC, comment #32: 

Hi Joel - I'm sorry I wasn't more specific. I happen to need an implementation of recvmsg() for a project I'm working on using an ESP32. I am specifically interested in the IP_PKTINFO feature, though I can do without an IPv6 implementation.

Is the patch you speak of in the source code area? I don't think I've ever patched code, so this will be a learning experience.

Thanks...

Michael Zimmers <mzimmers>
Sat 31 Mar 2018 04:35:40 AM UTC, comment #31: 

Michael,

Which part of the change are you inquiring about?

The remaining task I’d like to work on is an IPv6 version of IP_PKTINFO before closing this. I believe an implementation is available in patch #9554, but it needs separation from the large changeset that was posted there

Joel Cunningham <jcunningham>
Group Member
Fri 30 Mar 2018 08:49:28 PM UTC, comment #30: 

May I ask the status of this change?

Michael Zimmers <mzimmers>
Thu 27 Jul 2017 06:57:26 PM UTC, comment #29: 

No, it's fine like that. I'll just remove the "in progress" marker then. I'm trying to get a hold on what's missing for 2.1.0 and reading your post, it doesn't make sense to wait for this task to be finished.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 26 Jul 2017 09:34:02 PM UTC, comment #28: 

No update.  I'd still like to get the IPv6 version of IP_PKTINFO implemented, but haven't found time to get it done.  We can move that to a new issue if you'd like to close this one out.  recvmsg with control messages and IP_PKTINFO is ready to be used

Joel Cunningham <jcunningham>
Group Member
Wed 26 Jul 2017 07:41:01 PM UTC, comment #27: 

This is marked as "in progress", any update here or should we remove the marker?

Simon Goldschmidt <goldsimon>
Group administrator
Wed 26 Apr 2017 01:46:37 PM UTC, comment #26: 

Simon,

Thanks for clarifying the error.  I'm also fine with the added void .  RFC says the type should be unsigned char , but void should be fine since we are always casting to the real type (for CMSG_DATA)

I've also pushed a fix for ALIGN_H and ALIGN_D to address the potential problem on 16-bit arches where MEM_ALIGN is 2, but we need to be aligning to 4.  This change also ensures that there is no unaligned access even for systems that support it (x86)


Joel Cunningham <jcunningham>
Group Member
Wed 26 Apr 2017 06:45:06 AM UTC, comment #25: 

I've pushed a change adding a cast via void pointer (which is what Linux does :)

Simon Goldschmidt <goldsimon>
Group administrator
Wed 26 Apr 2017 06:19:03 AM UTC, comment #24: 

Whatever the problem with ALIGN_H and ALIGN_D is, the compiler error would not be fixed by changing that. The compiler just sees


struct in_pktinfo* pktinfo = (struct in_pktinfo*)(u8_t*)ptr;


It's the direct cast from u8_t pointer to struct pointer that is "bad". Using LWIP_ALIGNMENT_CAST() should fix this, but I'm not sure where to add that or what the standard thing to do here is. We should probably return a void pointer here instead of an u8_t pointer.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 25 Apr 2017 07:10:07 PM UTC, comment #23: 

Travis CI for clang is showing the follow error:


../../../../lwip/src/../test/unit/api/test_sockets.c:530:13: error: cast from 'u8_t *' (aka 'unsigned char *') to 'struct in_pktinfo *' increases required alignment from 1 to 4 [-Werror,-Wcast-align]
  pktinfo = (struct in_pktinfo*)CMSG_DATA(cmsg);
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../../lwip/src/../test/unit/api/test_sockets.c:536:10: error: cast from 'u8_t *' (aka 'unsigned char *') to 'struct cmsghdr *' increases required alignment from 1 to 4 [-Werror,-Wcast-align]
  cmsg = CMSG_NXTHDR(&msg, cmsg);
         ^~~~~~~~~~~~~~~~~~~~~~~
../../../../lwip/src/include/lwip/sockets.h:162:11: note: expanded from macro 'CMSG_NXTHDR'
          (struct cmsghdr *)((u8_t *)(cmsg) + \


I think we might have a problem with using MEM_ALIGNMENT for ALIGN_H and ALIGN_D.

I'm somewhat confused why the error is generated for clang on x86_64.  I think fno-sanitize=alignment is forcing no unaligned access on x86_64, right?  If so, why is MEM_ALIGNMENT set to 1 for unix port (ports\unix\lib\lwipopts.h or ports\unix\minimal\lwipopts.h, not sure which the unit test is using)?

Second, I could set ALIGN_H and ALIGN_D to 4 which would be correct for architectures with 4 byte alignment, but what about architectures with 2 byte alignment?  I don't have much experience with these architectures.  cmsg_len (type socklen_t) could end up being a u32 and in a 2 byte architecture, does this need to start on a 4 byte boundary?  If so, then using MEM_ALIGNMENT would not work.

If we ever add a cmsg data type that is 64-bits, we'll need to up the alignment to 8, even though MEM_ALIGNMENT would still be 4 on architectures like ARM.

Joel Cunningham <jcunningham>
Group Member
Tue 25 Apr 2017 02:49:02 PM UTC, comment #22: 

Patch committed in 2f117add7a8af07e483e1ee18af559f3e4e0d389.  Updating progress to 90%.  Remaining 10% is for IPv6 version of IP_PKTINFO (hopefully I can provide a patch later in the week)

Joel Cunningham <jcunningham>
Group Member
Tue 25 Apr 2017 02:05:41 PM UTC, comment #21: 


> It seemed minor to me since it was only a single check of: if (msg->msg_contrllen).


I rather thought about the 2nd indrection we get in standard receive because we create an (unused) struct msghdr. It's not a big deal, but if we find a decent way to separate it...

> Do you want me to go ahead and commit?


Yes :-)

Simon Goldschmidt <goldsimon>
Group administrator
Tue 25 Apr 2017 02:01:51 PM UTC, comment #20: 

We could move the msg_control code out of lwip_recvfrom_udp_raw() if we change the function so it can output the netbuf (which would then be processed in lwip_recvmsg).  It seemed minor to me since it was only a single check of: if (msg->msg_contrllen).

Do you want me to go ahead and commit?

Joel Cunningham <jcunningham>
Group Member
Tue 25 Apr 2017 08:20:27 AM UTC, comment #19: 

That looks much better. Although I'm still not convinced we need the msg_control code when calling lwip_recvfrom, but let's optimize that later...

Simon Goldschmidt <goldsimon>
Group administrator
Tue 25 Apr 2017 04:45:41 AM UTC, comment #18: 

New patch moves the flag to the netconn layer.  The netconn layer flag + using the flag for the existing LWIP_NETBUF_RECVINFO feature resulted in a much cleaner implementation, thanks for the suggestion :)

I went with defaulting netconn_alloc() to set NETCONN_FLAG_PKTINFO and then lwip_socket() to clear the flag.  This should retain the previous behavior when LWIP_NETBUF_RECVINFO was enabled

(file #40488)

Joel Cunningham <jcunningham>
Group Member
Wed 19 Apr 2017 08:06:36 PM UTC, comment #17: 

Ahh, now I see what you meant about ' enable NETCONN_FLAG_PKTINFO for netconns (add a function/define for that!)' that way we could maintain the existing LWIP_NETBUF_RECVINFO behavior.

I'll work on a new patch!

Joel Cunningham <jcunningham>
Group Member
Wed 19 Apr 2017 07:46:46 PM UTC, comment #16: 

Sorry for not being clear enough :-)
Yes, we would need the check in recv_udp. And we would need the netconn->flag for LWIP_NETBUF_RECVINFO (!=0). But we would store the info only if the flag PKTINFO is enabled in the netconn.
And that could be enabled by default to leave netconn behaviour as it was, and be disabled for sockets. But in the end, it's probably good enough to just disable it by default for all netconns & sockets and provide an API to enable it...

Simon Goldschmidt <goldsimon>
Group administrator
Wed 19 Apr 2017 07:40:20 PM UTC, comment #15: 

Thanks for the suggestion, I had also thought of doing a socket/netconn level flag, let me make sure I understand all the suggestions

> leave LWIP_NETBUF_RECVINFO as it was


I would still need to utilize the logic in recv_udp to store the address and the toaddr storage in netbuf.  Are you suggesting that I enable the toaddr storage all the time?  If I have access to toaddr and flags, I wouldn't need to do anything with LWIP_NETBUF_RECVINFO

> enable NETCONN_FLAG_PKTINFO for netconns (add a function/define for that!)


Add API to enable/disable PKTINFO.  If enabled at the netconn layer, toaddr and port are filled out in the netbuf

> disable NETCONN_FLAG_PKTINFO for sockets


By this you just mean it's off by default for sockets?  I would also think it would be off by default for netconns

Joel Cunningham <jcunningham>
Group Member
Wed 19 Apr 2017 07:13:55 PM UTC, comment #14: 

The SOF_PKTINFO flag is better off in struct netconn anyway. It is not used in core code at all.
Given that we could:
- make it NETCONN_FLAG_PKTINFO
- leave LWIP_NETBUF_RECVINFO as it was
- enable NETCONN_FLAG_PKTINFO for netconns (add a function/define for that!)
- disable NETCONN_FLAG_PKTINFO for sockets
- enable NETCONN_FLAG_PKTINFO if enabled via setsockopt(IP_PKTINFO)
- output PKTINFO if the netbuf has that information

Simon Goldschmidt <goldsimon>
Group administrator
Wed 19 Apr 2017 07:01:00 PM UTC, comment #13: 

 >  thing that looks strange is accessing the netconn's pcb without locking. Even if it's only a read access, chances are that the pcb already got deallocated. If not now, this might happen in the future. Isn't it enough to check the flag in the netbuf in this case?

Thanks for reminding me that PCB could be deallocated.  I think I can remove the check because your suggestion of relying on buf->flags having NETBUF_FLAG_DESTADDR set is enough.

I was thinking down the road when we add the IPv6 version, NETBUF_FLAG_DESTADDR would be set regardless of which version of PKTINFO socket option was set, but maybe checking the IP type via IP_IS_V4 or _IS_V6 is enough to determine which cmsg to output.  Maybe I can also check the domain in setsockopt and error if IP_PKTINFO is mixed with AF_INET6 (and vice versa for AF_NET with IPv6 option)

 > Shouldn't the new feature work with LWIP_NETBUF_RECVINFO==1, too?

I could make it work with this configuration as well, but was limiting the scope of the change.  The netbuf flags field would need to be enabled this configuration (currently it's not).  Also in relation to the above point about not accessing the PCB and checking NETBUF_FLAG_DESTADDR, with NETBUF_RECVINFO == 1, the flag would always bet set regardless of the IP_PKTINFO option, which would mean if a cmsg was provided during recvmsg, but IP_PKTINFO was off, the stack would still output PKTINFO in the cmsg.  Maybe this isn't a big deal since LWIP_NETBUF_RECVINFO == 1 always outputs the address

Joel Cunningham <jcunningham>
Group Member
Tue 18 Apr 2017 08:00:48 PM UTC, comment #12: 

Looks good. I can't comment on the corectness of the socket.h/.c additions though (CMSG...) without reading into the specs. But I guess that doesn't really matter. If it should be wrong, we can always fix it later.

I have two questions/comments though:
1) A thing that looks strange is accessing the netconn's pcb without locking. Even if it's only a read access, chances are that the pcb already got deallocated. If not now, this might happen in the future. Isn't it enough to check the flag in the netbuf in this case?
2) Shouldn't the new feature work with LWIP_NETBUF_RECVINFO==1, too?

Simon Goldschmidt <goldsimon>
Group administrator
Sat 15 Apr 2017 05:08:07 PM UTC, comment #11: 

Attached is patch implementing CMSG infrastructure (only for recvmsg) and IP_PKTINFO socket option (including unit test)

I have a couple of things to discuss about the change, patch contains current approach:

1. For struct in_pktinfo, I went with the netbsd usage rather than Linux.  My thoughts here are that Linux has the additional ipi_spec_dst, which I believe will be set to the source address of the interface that received the packet.  Since this was going to be an additional lookup once we are outside of the LwIP core context, it didn't seem necessary.  Applications can still get this address from the interface index (we can implement SIOCGIFADDR if needed)

2. I re-used the LWIP_NETBUF_RECVINFO feature (rather than create a new option) to record the destination when the datagram is received, but I made a modification to the option.  I added a new subset of behavior.  If LWIP_NETBUF_RECVINFO is 2, the destination and port are only recorded when IP_PKTINFO is enabled.  This preserves the old API behavior of LWIP_NETBUF_RECVINFO == 1 and doesn't suffer the performance hit of recording on every received datagram.  I also enabled the netbuf flags for this case so we can check that the address was recorded since it's now dependent on IP_PKTINFO being enabled

3. I added an IP level so_option for PKTINFO (called SOF_PKTINFO).  This differs a little from the other options because it doesn't directly map from the SO_ equvialent because IP_PKTINFO if an IP level option.  I didn't see a better way to record this setting without introducing a separate flag

4. I changed lwip_recvfrom_udp_raw() to take struct msghdr because I need msg_control, msg_controllen, msg_flags and almost all members of the msghdr were being passed individually.  This also means that lwip_recvfrom() is more or less just calling recvmsg because it sets up struct msghdr

Thanks :)

(file #40412)

Joel Cunningham <jcunningham>
Group Member
Thu 13 Apr 2017 03:39:42 PM UTC, comment #10: 

RFC 3542, section 20 contains the specification for control messages (AKA Ancillary Data):

https://tools.ietf.org/html/rfc3542#section-20

Joel Cunningham <jcunningham>
Group Member
Fri 17 Mar 2017 10:22:53 PM UTC, comment #9: 

Socket unit tests now utilize recvmsg for both UDP and TCP.  So far no bugs in the implementation :)

Joel Cunningham <jcunningham>
Group Member
Thu 16 Mar 2017 10:21:09 PM UTC, comment #8: 

Awesome, great :)  I'll start playing with it.  I can work on moving my sendmsg tests and convert them to using loopback

Joel Cunningham <jcunningham>
Group Member
Thu 16 Mar 2017 09:57:19 PM UTC, comment #7: 


> Do we have any mechanisms for testing the API yet?


I just tried to get started with this, have a look at test/unit/api and test/unit/arch.

I'd be happy to see more tests there, feel free to add them. That would be a much better place than contrib/apps/socket_examples (where the remote host is missing)

Simon Goldschmidt <goldsimon>
Group administrator
Tue 14 Mar 2017 06:44:20 PM UTC, comment #6: 


> Do we have any mechanisms for testing the API yet?


Unfortunately not. I'd love to have unit tests for sockets and netconn, but that would need callback driven test implementation of the sys_arch layer.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 14 Mar 2017 06:55:42 AM UTC, comment #5: 

That would be great. I haven't started and don't know when I would have...

Simon Goldschmidt <goldsimon>
Group administrator
Mon 13 Mar 2017 10:09:57 PM UTC, comment #4: 

I can give adding IP_PKTINFO and CMSG support a go if you hadn't already started on it.  I plan on using the feature in my products and have also implemented these APIs in a proprietary stack a couple years back.

Do we have any mechanisms for testing the API yet?  A longer term project, but maybe a sockets unit test would be helpful (we've been making more changes in sockets since 2.0.0 and there's probably some bugs)

Joel Cunningham <jcunningham>
Group Member
Mon 13 Mar 2017 09:34:19 PM UTC, comment #3: 

The actual recvmsg function is implemented, however, IP_PKTINFO/in_pktinfo is still missing (msg_control is not touched at all).

Feel free to add patches here :-)

Simon Goldschmidt <goldsimon>
Group administrator
Wed 23 Nov 2016 03:06:28 PM UTC, comment #2: 

The IPv6 version of all this stuff is separate and (fortunately) standardized in RFC 3542.

David van Moolenbroek <dcvmoole>
Wed 23 Nov 2016 02:51:25 PM UTC, comment #1: 

Glad to see this as a task, recvmsg has been on my backlog for a while :)

I believe there is no open group standards for the message types and it ends up being fairly platform dependent.

Linux, NetBSD, and Windows use IP_PKTINFO, but struct in_pktinfo is different on the platforms.

Linux
NetBSD
Windows

FreeBSD has IP_RECVDSTADDR

For LwIP, I guess we get to pick which option and if IP_PKTINFO, the definition of struct in_pktinfo

Joel Cunningham <jcunningham>
Group Member
Wed 23 Nov 2016 12:01:25 PM UTC, original submission:  

Together with IP_PKTINFO option (is this standardized?), recvmsg() can give us the destination address of UDP datagrams received on an ANY socket...

Simon Goldschmidt <goldsimon>
Group administrator

 

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

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mzimmers (Posted a comment)
  • -email is unavailable- added by dcvmoole (Posted a comment)
  • -email is unavailable- added by jcunningham (Posted a comment)
  • -email is unavailable- added by goldsimon (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 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-07-13 jcunningham Assigned tojcunningham None
    2017-07-27 goldsimon StatusIn Progress None
    2017-04-26 goldsimon Percent Complete50% 90%
    2017-04-25 jcunningham Percent Complete90% 50%
    2017-04-25 jcunningham Percent Complete50% 90%
    2017-04-25 jcunningham Attached File- Added 0001-sockets-task-14247-add-CMSG-and-IP_PKTINFO-conn-flag.patch, #40488
    2017-04-15 jcunningham Attached File- Added 0001-sockets-task-14247-add-CMSG-and-IP_PKTINFO.patch, #40412
    2017-04-13 jcunningham Assigned toNone jcunningham
    2017-03-13 goldsimon StatusNone In Progress
        Percent Complete0% 50%
        Summarysocket API: implement recvmsg socket API: implement recvmsg msg_control support

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code