Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gnrc_ipv6: Forward multicast packets even if they are registered with the receiving netif #4528

Closed

Conversation

DipSwitch
Copy link
Member

Currently if you send a broadcast to ff05::1 for example a routing node doesn't forward the packed if the router itself has the address assigned to it's receiving interface.

Thoughts on the subject are welcome :)

@DipSwitch DipSwitch added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Dec 20, 2015
static inline bool _pkt_none_local_mcast(ipv6_hdr_t *hdr)
{
return (hdr->dst->u8[0] == 0xff) &&
(hdr->dst->u8[1] >= IPV6_ADDR_MCAST_SCP_LINK_LOCAL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ignores the flags of a multicast address. ff12::1 would also be a legal link-local address.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Learned something new. Fixed it

@miri64
Copy link
Member

miri64 commented Dec 20, 2015

Fixes #4527

@miri64
Copy link
Member

miri64 commented Dec 20, 2015

Another problem is, that we currently don't implement MLP, so this configuration will most likely flood a network.

@DipSwitch
Copy link
Member Author

Would I also need to change the >= to > since forwarding link-local speaks against itself...

@miri64
Copy link
Member

miri64 commented Dec 20, 2015

Yes, thanks for pointing that out

@miri64
Copy link
Member

miri64 commented Dec 20, 2015

oh and while we're are at nitpicky stuff: the module you're changing is gnrc_ipv6 not gnrc_sixlowpan

@miri64 miri64 changed the title gnrc_sixlowpan: Forward multicast packets even if they are registered with the receiving netif gnrc_ipv6: Forward multicast packets even if they are registered with the receiving netif Dec 20, 2015
@DipSwitch
Copy link
Member Author

What does MLP stand for? Multinode Link Layer Protocol?

And sorry about that, first thought it was 6Low specific.

@cgundogan
Copy link
Member

What does MLP stand for? Multinode Link Layer Protocol?

I believe @authmillenon was referring to MLD

@miri64
Copy link
Member

miri64 commented Dec 20, 2015

Yes, sorry for the confusion.

@DipSwitch
Copy link
Member Author

DipSwitch commented Dec 20, 2015 via email

@OlegHahm OlegHahm added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Feb 27, 2016
@OlegHahm
Copy link
Member

@authmillenon, what's the state here?

@miri64
Copy link
Member

miri64 commented Feb 28, 2016

I have not tested it yet, but code-wise it looks alright. Will test at the Hack'n'ACK :-)

@miri64
Copy link
Member

miri64 commented Mar 1, 2016

Wait a minute: what if the router has the address assigned to one of its interfaces. Then it won't forward it though it might make sense.

@miri64
Copy link
Member

miri64 commented Mar 6, 2016

@DipSwitch ping?

@DipSwitch
Copy link
Member Author

Whit this PR it would forward it? That was my intention at leased. But maybe I misunderstood what you said.

@miri64
Copy link
Member

miri64 commented Mar 7, 2016

Ah okay, yes. I forgot about the ||... But shouldn't it also receive it?

@DipSwitch
Copy link
Member Author

Yeah, I thought it tries to process it anyway. Will fix tonight. :)
On 7 Mar 2016 11:36, "Martine Lenders" notifications@github.com wrote:

Ah okay, yes. I forgot about the ||... But shouldn't it also receive it?


Reply to this email directly or view it on GitHub
#4528 (comment).

@DipSwitch
Copy link
Member Author

Rebased, squashed the rest, and fixed to also processing the packet ourselfs part.

@OlegHahm OlegHahm added this to the Release 2016.04 milestone Mar 25, 2016
@miri64
Copy link
Member

miri64 commented Mar 29, 2016

Will test tonight.

@@ -738,6 +738,12 @@ static inline bool _pkt_not_for_me(kernel_pid_t *iface, ipv6_hdr_t *hdr)
}
}

static inline bool _pkt_none_local_mcast(ipv6_hdr_t *hdr)
{
return (hdr->dst->u8[0] == 0xff) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/dst->u8/dst.u8/

@DipSwitch
Copy link
Member Author

Addressed, rebased, squashed, pushed

@miri64
Copy link
Member

miri64 commented Mar 29, 2016

When I try to ping an added ff0e::1 on another node the receiver hard faulted due to a failed assertion in packet buffer.

> ifconfig 6 add ff0e::1
ifconfig 6 add ff0e::1
success: added ff0e::1/64 to interface 6
> 0x49dd7a1
*** RIOT kernel panic:
FAILED ASSERTION.

    pid | name                 | state    Q | pri | stack ( used) | location   
      1 | idle                 | pending  Q |  15 |  8192 ( 6240) | 0x807c2c0 
      2 | main                 | pending  Q |   7 | 12288 ( 9312) | 0x80792c0 
      3 | pktdump              | bl rx    _ |   6 | 12288 ( 9312) | 0x808aac0 
      4 | ipv6                 | running  Q |   4 |  8192 ( 6240) | 0x8086d60 
      5 | udp                  | bl rx    _ |   5 |  8192 ( 6240) | 0x808dae0 
      6 | gnrc_netdev2_tap     | bl rx    _ |   4 |  8192 ( 6240) | 0x8084d40 
        | SUM                  |            |     | 57344 (43584)

*** halted.


Program received signal SIGSEGV, Segmentation fault.
0x00000081 in ?? ()
(gdb) where
#0  0x00000081 in ?? ()
#1  0x00000001 in ?? ()
#2  0x0806d374 in ?? ()
#3  0x08058ff0 in _release_error_locked (pkt=0x50, err=0) at /home/martine/Repositories/RIOT-OS/RIOT/sys/net/gnrc/pktbuf_static/gnrc_pktbuf_static.c:224
#4  0x080590a3 in gnrc_pktbuf_release_error (pkt=0x80892a0 <_pktbuf>, err=0)
    at /home/martine/Repositories/RIOT-OS/RIOT/sys/net/gnrc/pktbuf_static/gnrc_pktbuf_static.c:243
#5  0x0804fece in gnrc_pktbuf_release (pkt=0x80892a0 <_pktbuf>) at /home/martine/Repositories/RIOT-OS/RIOT/sys/include/net/gnrc/pktbuf.h:163
#6  0x08051002 in _receive (pkt=0x80892a0 <_pktbuf>) at /home/martine/Repositories/RIOT-OS/RIOT/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c:889
#7  0x08050393 in _event_loop (args=0x0) at /home/martine/Repositories/RIOT-OS/RIOT/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c:237
#8  0xf7ddef0b in makecontext () from /usr/lib32/libc.so.6
#9  0x00000000 in ?? ()
(gdb) 

@DipSwitch
Copy link
Member Author

Last time I tried this was 20 December probably, and I don't have tranceivers at home yet. Moving to next release. :)

@OlegHahm
Copy link
Member

OlegHahm commented Apr 1, 2016

Hm, since this is a bugfix, we can still merge - and I would like to see this problem solved.

@@ -738,6 +738,12 @@ static inline bool _pkt_not_for_me(kernel_pid_t *iface, ipv6_hdr_t *hdr)
}
}

static inline bool _pkt_none_local_mcast(ipv6_hdr_t *hdr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"none" -> "non" or "not"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

@miri64
Copy link
Member

miri64 commented May 31, 2016

Please rebase.

@miri64
Copy link
Member

miri64 commented Jun 6, 2016

Please rebase, we need this fix!

@kYc0o kYc0o removed the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Jul 12, 2016
@kYc0o
Copy link
Contributor

kYc0o commented Jul 12, 2016

Can this fix come into the release? Maybe is it related to #5596 ?

@OlegHahm
Copy link
Member

If @DipSwitch is unavailable right now, we could adopt it.

@kYc0o
Copy link
Contributor

kYc0o commented Jul 18, 2016

Can you @OlegHahm ?

@OlegHahm
Copy link
Member

Not this week.

@kYc0o
Copy link
Contributor

kYc0o commented Jul 18, 2016

OK, so I need to postpone it.

@kYc0o kYc0o modified the milestones: Release 2016.10, Release 2016.07 Jul 18, 2016
@miri64
Copy link
Member

miri64 commented Aug 5, 2016

I go ahead and adapt it myself. Can you review @kYc0o?

@kYc0o
Copy link
Contributor

kYc0o commented Aug 5, 2016

I can try to test, I'm supposed to be off today but I'll have some time.

@miri64
Copy link
Member

miri64 commented Aug 5, 2016

kk

@miri64
Copy link
Member

miri64 commented Aug 5, 2016

Need not be today ;-)

@kYc0o
Copy link
Contributor

kYc0o commented Aug 5, 2016

Let's do it then ;-)

@miri64
Copy link
Member

miri64 commented Aug 5, 2016

See #5729.

@miri64 miri64 closed this Aug 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants