patchlwIP - A Lightweight TCP/IP stack - Patches: patch #9169, Allows multicast packets through...

 
 

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

patch #9169: Allows multicast packets through udp input layer

Submitter:  Alex Moran <alex081293>
Submitted:  Mon 21 Nov 2016 07:49:51 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Invalid Privacy:  Public
Assigned to:  None Open/Closed:  Closed
Planned Release:  None

Jump to the original submission

Wed 23 Nov 2016 02:28:35 PM UTC, comment #8: 


> Am I missing something?


You really are confusing bind and connect. If you want to receive multicast packets sent to 239.1.80.1, you bind the socket to 239.1.80.1 (along with the destination port), in addition to joining the multicast group. If you want to receive such packets sent from 192.168.1.2 or 192.168.1.3 only, you will have to perform such source-IP checks in the application - but that does not seem to be what you want anyway. In other words, to achieve what you originally wanted, you can simply do this:

struct ip_addr multicast_addr;
IP4_ADDR(&multicast_addr, 239, 1, 80, 1);
int mPort= 12032;

udp_bind(this->pcb, &multicast_addr, mPort);

David van Moolenbroek <dcvmoole>
Tue 22 Nov 2016 09:06:10 PM UTC, comment #7: 


> You (OP) mix up remote address with local address. udp_connect() tells the stack something about the remote address. This is the SRC in incoming packets. You compare it against DST of these packets but only for multicast IPs. That does not make sense at all!


It does make sense. The incoming IP header of a multicast packet has the DST as the multicast address and the src as the senders IP. If we have "connected" the pcb to a multicast address, we want to process at the application layer all packets that are sent to 239.1.80.1. So if the connected IP address is the PCB's remote pcb is multicast, we want to compare the DST address with the remote PCB.

To take an example of an ip's from 192.168.1.2 and 192.168.1.3 are sending packets to Multicast Address 239.1.80.1, and the receiver (us) connects to a pcb to address 239.1.80.1. We want to receive all the packets sent to 239.1.80.1 on one PCB.

The multicast packet will have a src of 192.168.1.2/3 with a destination of 239.1.80.1, and the remote PCB will be looking for all traffic going to 239.1.80.1.

Currently, it is only comparing the pcb->remote_ip (239.1.80.1) with the src ip (192.168.1.2/3). If the remote IP is 239.1.80.1, a multicast address, we want to compare the destination IP (239.1.80.1) with our remote IP (also 239.1.80.1).

If the connected remote PCB is not a multicast address, we want to do
           ip_addr_cmp(&(pcb->remote_ip), &current_iphdr_src)
Otherwise, we'd want
           ip_addr_cmp(&(pcb->remote_ip), &current_iphdr_dest)

Without the patch, I'm not sure how it passes the packet up the application layer.

Am I missing something?

Alex Moran <alex081293>
Tue 22 Nov 2016 08:03:52 PM UTC, comment #6: 

Ooops, haven't read comment #4 before sending #5

Simon Goldschmidt <goldsimon>
Group administrator
Tue 22 Nov 2016 07:39:46 PM UTC, comment #5: 

To you both: it would have been better to keep the discussion about patch #9165 there, these long off-topic posts confuse me ;-)

As to the multicast traffic:

You (OP) mix up remote address with local address. udp_connect() tells the stack something about the remote address. This is the SRC in incoming packets. You compare it against DST of these packets but only for multicast IPs. That does not make sense at all!

That being said, I would have thought you
a) bind to the multicast IP and
b) use setsockopt(IP_MULTICAST_IF) to select a netif to bind to if you have more than one

Bug #49662 (which is fairly new) suggests this doesn't work, but if that's true, we'll fix it ;-)


In the end, we don't want to reinvent the wheel with lwIP, just make it more lightweight ;-)

Simon Goldschmidt <goldsimon>
Group administrator
Tue 22 Nov 2016 07:37:36 PM UTC, comment #4: 

[...]
Why not have it configured to be able to handle both cases?

Because you can easily achieve this by filtering the destination address as first thing in your reception routine.

Moving this capability to lwIP is possible and will make your application smaller and easier.

But:
- it will make all other applications slightly slower due to additional checks
- will make the udp code more complicated / more difficult to maintain for a corner-case
- will add an unknown/non-standard semantics to udp_connect() noone knows about and noone expects to work like this
- we are in a lightweight stack - we do not want so solve every possible application case in the stack

Last, especially patch #9169 introduces strange behavior by comparing pcb->remote_ip with a DESTINATION ip instead of source ip

Dirk Ziegelmeier <dziegel>
Group administrator
Tue 22 Nov 2016 06:03:31 PM UTC, comment #3: 


> 2) I only want to receive from the specified remote IP address/port.

   Shouldn't this be possible but not care about the port? I ONLY want data from that IP address, I want to connect to that device...it's much like UDP bind (in regards to changing semantics), where port 0 is an invalid port signifies you really don't care what port you open up on. Why not have it configured to be able to handle both cases?

The IP/UDP layer should be able to be configured in a way so that the application layer will only be given data that it wants. This is a solution so it's guaranteed. If it's not necessary to put the burden on the application, then it should be done on the UDP layer.

In regards to the multicast traffic, you're saying it is impossible to receive a multicast packet without receiving all incoming data to that port, this doesn't seem right? There is no way to filter out multicast traffic on the udp layer?

In regards to both patches, I can't think of a way to misuse/abuse the library in a way that you'd be receiving packets that are unexpected or breaks functionality. By not adding it, it's asking the programmer to know the limitations of the library and add basic features themselves in the application layer.

Alex Moran <alex081293>
Tue 22 Nov 2016 01:31:27 PM UTC, comment #2: 

Simply don't call udp_connect() and you will receive all multicast traffic on that PCB automatically.

udp_connect() means:
1) If I call udp_send() somewhere, it should go to the specified remote address/port.
2) I only want to receive from the specified remote IP address/port.


Note:
I'm currently thinking about reverting your last patch, because you can get your desired behavior in this case by also not calling udp_connect() and comparing the source IP as the first thing to do in your udp_recv() function.

Dirk Ziegelmeier <dziegel>
Group administrator
Mon 21 Nov 2016 07:56:00 PM UTC, comment #1: 

Of course an igmp_joingroup would also be required to the example code I added.

Alex Moran <alex081293>
Mon 21 Nov 2016 07:49:51 PM UTC, original submission:  

If user opens a raw socket to a multicast address, the current udp layer drops the packet because it's only looking if remote ip address is any value or if the pcb remote address is equal to the packet source address. In the case of multicast traffic, the packet source is irrelevant the critical info is if the packet destination is equal to the pcb remote ip.

I propose adding an extra condition on lines 291-295:
  ...
  if (((pcb->remote_port == src) || (pcb->remote_port == 0)) &&
     (ip_addr_isany_val(pcb->remote_ip) ||
     ip_addr_cmp(&pcb->remote_ip, ip_current_src_addr())
  // PATCH - Allow multicast packets
#if LWIP_IGMP
     || (ip_addr_ismulticast(&(pcb->remote_ip)) && (ip_addr_cmp(&(pcb->remote_ip), &current_iphdr_dest)))
#endif
   )) {
   ...

So that the device can open a socket like so:

   ...
   struct ip_addr multicast_addr;
   IP4_ADDR(&multicast_addr,  239, 1, 80, 1);
   int mPort= 12032;

   udp_bind(this->pcb, IP_ADDR_ANY, mPort);
   udp_connect(this->pcb, &(multicast_addr), 0);
   ...


And receive all multicast traffic aimed at address 239.1.80.1 to port 12032 regardless the ip src.

Alex Moran <alex081293>

 

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

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2016-11-22 dziegel StatusNone Invalid
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code