Skip to content

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

Closed
DipSwitch wants to merge 1 commit intoRIOT-OS:masterfrom
DipSwitch:pr/fix_6lo_multicast_route
Closed

gnrc_ipv6: Forward multicast packets even if they are registered with the receiving netif#4528
DipSwitch wants to merge 1 commit intoRIOT-OS:masterfrom
DipSwitch:pr/fix_6lo_multicast_route

Conversation

@DipSwitch
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

miri64 commented Dec 20, 2015

Fixes #4527

@miri64
Copy link
Copy Markdown
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
Copy Markdown
Member Author

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

@miri64
Copy link
Copy Markdown
Member

miri64 commented Dec 20, 2015

Yes, thanks for pointing that out

@miri64
Copy link
Copy Markdown
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
Copy Markdown
Member Author

What does MLP stand for? Multinode Link Layer Protocol?

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

@cgundogan
Copy link
Copy Markdown
Member

What does MLP stand for? Multinode Link Layer Protocol?

I believe @authmillenon was referring to MLD

@miri64
Copy link
Copy Markdown
Member

miri64 commented Dec 20, 2015

Yes, sorry for the confusion.

@DipSwitch
Copy link
Copy Markdown
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
Copy Markdown
Member

@authmillenon, what's the state here?

@miri64
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

miri64 commented Mar 6, 2016

@DipSwitch ping?

@DipSwitch
Copy link
Copy Markdown
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
Copy Markdown
Member

miri64 commented Mar 7, 2016

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

@DipSwitch
Copy link
Copy Markdown
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 DipSwitch force-pushed the pr/fix_6lo_multicast_route branch from 8f4a78b to be18502 Compare March 24, 2016 20:49
@DipSwitch
Copy link
Copy Markdown
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
Copy Markdown
Member

miri64 commented Mar 29, 2016

Will test tonight.


static inline bool _pkt_none_local_mcast(ipv6_hdr_t *hdr)
{
return (hdr->dst->u8[0] == 0xff) &&
Copy link
Copy Markdown
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 DipSwitch force-pushed the pr/fix_6lo_multicast_route branch from 345e9ec to 203b147 Compare March 29, 2016 20:33
@DipSwitch
Copy link
Copy Markdown
Member Author

Addressed, rebased, squashed, pushed

@miri64
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

}
}

static inline bool _pkt_none_local_mcast(ipv6_hdr_t *hdr)
Copy link
Copy Markdown
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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed.

@DipSwitch DipSwitch force-pushed the pr/fix_6lo_multicast_route branch from 203b147 to 88532c0 Compare April 17, 2016 15:06
@miri64
Copy link
Copy Markdown
Member

miri64 commented May 31, 2016

Please rebase.

@miri64
Copy link
Copy Markdown
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
Copy Markdown
Contributor

kYc0o commented Jul 12, 2016

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

@OlegHahm
Copy link
Copy Markdown
Member

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

@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented Jul 18, 2016

Can you @OlegHahm ?

@OlegHahm
Copy link
Copy Markdown
Member

Not this week.

@kYc0o
Copy link
Copy Markdown
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
Copy Markdown
Member

miri64 commented Aug 5, 2016

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

@kYc0o
Copy link
Copy Markdown
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
Copy Markdown
Member

miri64 commented Aug 5, 2016

kk

@miri64
Copy link
Copy Markdown
Member

miri64 commented Aug 5, 2016

Need not be today ;-)

@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented Aug 5, 2016

Let's do it then ;-)

@miri64
Copy link
Copy Markdown
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