* [PATCH net-next] ipv6: send Change Status Report after DAD is completed
@ 2014-01-15 19:10 Flavio Leitner
2014-01-16 11:21 ` Daniel Borkmann
0 siblings, 1 reply; 4+ messages in thread
From: Flavio Leitner @ 2014-01-15 19:10 UTC (permalink / raw)
To: netdev; +Cc: Hideaki YOSHIFUJI, Hannes Frederic Sowa, Flavio Leitner
The RFC 3810 defines two type of messages for multicast
listeners. The "Current State Report" message, as the name
implies, refreshes the *current* state to the querier.
Since the querier sends Query messages periodically, there
is no need to retransmit the report.
On the other hand, any change should be reported immediately
using "State Change Report" messages. Since it's an event
triggered by a change and that it can be affected by packet
loss, the rfc states it should be retransmitted [RobVar] times
to make sure routers will receive timely.
Currently, we are sending "Current State Reports" after
DAD is completed. Before that, we send messages using
unspecified address (::) which should be silently discarded
by routers.
This patch changes to send "State Change Report" messages
after DAD is completed fixing the behavior to be RFC compliant
and also to pass TAHI IPv6 testsuite.
Signed-off-by: Flavio Leitner <fbl@redhat.com>
---
net/ipv6/mcast.c | 64 ++++++++++++++++++++++++++++++++++----------------------
1 file changed, 39 insertions(+), 25 deletions(-)
diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index 99cd65c..8ac17f5 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -1493,7 +1493,7 @@ static struct sk_buff *add_grhead(struct sk_buff *skb, struct ifmcaddr6 *pmc,
skb_tailroom(skb)) : 0)
static struct sk_buff *add_grec(struct sk_buff *skb, struct ifmcaddr6 *pmc,
- int type, int gdeleted, int sdeleted)
+ int type, int gdeleted, int sdeleted, int crsend)
{
struct inet6_dev *idev = pmc->idev;
struct net_device *dev = idev->dev;
@@ -1585,7 +1585,7 @@ empty_source:
if (type == MLD2_ALLOW_NEW_SOURCES ||
type == MLD2_BLOCK_OLD_SOURCES)
return skb;
- if (pmc->mca_crcount || isquery) {
+ if (pmc->mca_crcount || isquery || crsend) {
/* make sure we have room for group header */
if (skb && AVAILABLE(skb) < sizeof(struct mld2_grec)) {
mld_sendpack(skb);
@@ -1602,6 +1602,28 @@ empty_source:
return skb;
}
+static void mld_send_initial_cr(struct inet6_dev *idev)
+{
+ struct sk_buff *skb;
+ struct ifmcaddr6 *pmc;
+ int type;
+
+ skb = NULL;
+ read_lock_bh(&idev->lock);
+ for (pmc=idev->mc_list; pmc; pmc=pmc->next) {
+ spin_lock_bh(&pmc->mca_lock);
+ if (pmc->mca_sfcount[MCAST_EXCLUDE])
+ type = MLD2_CHANGE_TO_EXCLUDE;
+ else
+ type = MLD2_CHANGE_TO_INCLUDE;
+ skb = add_grec(skb, pmc, type, 0, 0, 1);
+ spin_unlock_bh(&pmc->mca_lock);
+ }
+ read_unlock_bh(&idev->lock);
+ if (skb)
+ mld_sendpack(skb);
+}
+
static void mld_send_report(struct inet6_dev *idev, struct ifmcaddr6 *pmc)
{
struct sk_buff *skb = NULL;
@@ -1617,7 +1639,7 @@ static void mld_send_report(struct inet6_dev *idev, struct ifmcaddr6 *pmc)
type = MLD2_MODE_IS_EXCLUDE;
else
type = MLD2_MODE_IS_INCLUDE;
- skb = add_grec(skb, pmc, type, 0, 0);
+ skb = add_grec(skb, pmc, type, 0, 0, 0);
spin_unlock_bh(&pmc->mca_lock);
}
} else {
@@ -1626,7 +1648,7 @@ static void mld_send_report(struct inet6_dev *idev, struct ifmcaddr6 *pmc)
type = MLD2_MODE_IS_EXCLUDE;
else
type = MLD2_MODE_IS_INCLUDE;
- skb = add_grec(skb, pmc, type, 0, 0);
+ skb = add_grec(skb, pmc, type, 0, 0, 0);
spin_unlock_bh(&pmc->mca_lock);
}
read_unlock_bh(&idev->lock);
@@ -1671,13 +1693,13 @@ static void mld_send_cr(struct inet6_dev *idev)
if (pmc->mca_sfmode == MCAST_INCLUDE) {
type = MLD2_BLOCK_OLD_SOURCES;
dtype = MLD2_BLOCK_OLD_SOURCES;
- skb = add_grec(skb, pmc, type, 1, 0);
- skb = add_grec(skb, pmc, dtype, 1, 1);
+ skb = add_grec(skb, pmc, type, 1, 0, 0);
+ skb = add_grec(skb, pmc, dtype, 1, 1, 0);
}
if (pmc->mca_crcount) {
if (pmc->mca_sfmode == MCAST_EXCLUDE) {
type = MLD2_CHANGE_TO_INCLUDE;
- skb = add_grec(skb, pmc, type, 1, 0);
+ skb = add_grec(skb, pmc, type, 1, 0, 0);
}
pmc->mca_crcount--;
if (pmc->mca_crcount == 0) {
@@ -1708,8 +1730,8 @@ static void mld_send_cr(struct inet6_dev *idev)
type = MLD2_ALLOW_NEW_SOURCES;
dtype = MLD2_BLOCK_OLD_SOURCES;
}
- skb = add_grec(skb, pmc, type, 0, 0);
- skb = add_grec(skb, pmc, dtype, 0, 1); /* deleted sources */
+ skb = add_grec(skb, pmc, type, 0, 0, 0);
+ skb = add_grec(skb, pmc, dtype, 0, 1, 0); /* deleted sources */
/* filter mode changes */
if (pmc->mca_crcount) {
@@ -1717,7 +1739,7 @@ static void mld_send_cr(struct inet6_dev *idev)
type = MLD2_CHANGE_TO_EXCLUDE;
else
type = MLD2_CHANGE_TO_INCLUDE;
- skb = add_grec(skb, pmc, type, 0, 0);
+ skb = add_grec(skb, pmc, type, 0, 0, 0);
pmc->mca_crcount--;
}
spin_unlock_bh(&pmc->mca_lock);
@@ -1825,27 +1847,19 @@ err_out:
goto out;
}
-static void mld_resend_report(struct inet6_dev *idev)
+static void mld_resend_cr(struct inet6_dev *idev)
{
- if (MLD_V1_SEEN(idev)) {
- struct ifmcaddr6 *mcaddr;
- read_lock_bh(&idev->lock);
- for (mcaddr = idev->mc_list; mcaddr; mcaddr = mcaddr->next) {
- if (!(mcaddr->mca_flags & MAF_NOREPORT))
- igmp6_send(&mcaddr->mca_addr, idev->dev,
- ICMPV6_MGM_REPORT);
- }
- read_unlock_bh(&idev->lock);
- } else {
- mld_send_report(idev, NULL);
- }
+ if (MLD_V1_SEEN(idev))
+ return;
+
+ mld_send_initial_cr(idev);
}
void ipv6_mc_dad_complete(struct inet6_dev *idev)
{
idev->mc_dad_count = idev->mc_qrv;
if (idev->mc_dad_count) {
- mld_resend_report(idev);
+ mld_resend_cr(idev);
idev->mc_dad_count--;
if (idev->mc_dad_count)
mld_dad_start_timer(idev, idev->mc_maxdelay);
@@ -1856,7 +1870,7 @@ static void mld_dad_timer_expire(unsigned long data)
{
struct inet6_dev *idev = (struct inet6_dev *)data;
- mld_resend_report(idev);
+ mld_resend_cr(idev);
if (idev->mc_dad_count) {
idev->mc_dad_count--;
if (idev->mc_dad_count)
--
1.8.4.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH net-next] ipv6: send Change Status Report after DAD is completed
2014-01-15 19:10 [PATCH net-next] ipv6: send Change Status Report after DAD is completed Flavio Leitner
@ 2014-01-16 11:21 ` Daniel Borkmann
2014-01-16 12:21 ` Flavio Leitner
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Borkmann @ 2014-01-16 11:21 UTC (permalink / raw)
To: Flavio Leitner; +Cc: netdev, Hideaki YOSHIFUJI, Hannes Frederic Sowa
On 01/15/2014 08:10 PM, Flavio Leitner wrote:
> The RFC 3810 defines two type of messages for multicast
> listeners. The "Current State Report" message, as the name
> implies, refreshes the *current* state to the querier.
> Since the querier sends Query messages periodically, there
> is no need to retransmit the report.
>
> On the other hand, any change should be reported immediately
> using "State Change Report" messages. Since it's an event
> triggered by a change and that it can be affected by packet
> loss, the rfc states it should be retransmitted [RobVar] times
> to make sure routers will receive timely.
>
> Currently, we are sending "Current State Reports" after
> DAD is completed. Before that, we send messages using
> unspecified address (::) which should be silently discarded
> by routers.
>
> This patch changes to send "State Change Report" messages
> after DAD is completed fixing the behavior to be RFC compliant
> and also to pass TAHI IPv6 testsuite.
>
> Signed-off-by: Flavio Leitner <fbl@redhat.com>
> ---
> net/ipv6/mcast.c | 64 ++++++++++++++++++++++++++++++++++----------------------
> 1 file changed, 39 insertions(+), 25 deletions(-)
>
> diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
> index 99cd65c..8ac17f5 100644
> --- a/net/ipv6/mcast.c
> +++ b/net/ipv6/mcast.c
> @@ -1493,7 +1493,7 @@ static struct sk_buff *add_grhead(struct sk_buff *skb, struct ifmcaddr6 *pmc,
> skb_tailroom(skb)) : 0)
>
> static struct sk_buff *add_grec(struct sk_buff *skb, struct ifmcaddr6 *pmc,
> - int type, int gdeleted, int sdeleted)
> + int type, int gdeleted, int sdeleted, int crsend)
Maybe we should convert the last three to 'bool' at some point in time?
Nit: argument indent
> {
> struct inet6_dev *idev = pmc->idev;
> struct net_device *dev = idev->dev;
> @@ -1585,7 +1585,7 @@ empty_source:
> if (type == MLD2_ALLOW_NEW_SOURCES ||
> type == MLD2_BLOCK_OLD_SOURCES)
> return skb;
> - if (pmc->mca_crcount || isquery) {
> + if (pmc->mca_crcount || isquery || crsend) {
> /* make sure we have room for group header */
> if (skb && AVAILABLE(skb) < sizeof(struct mld2_grec)) {
> mld_sendpack(skb);
> @@ -1602,6 +1602,28 @@ empty_source:
> return skb;
> }
>
> +static void mld_send_initial_cr(struct inet6_dev *idev)
> +{
> + struct sk_buff *skb;
> + struct ifmcaddr6 *pmc;
> + int type;
> +
> + skb = NULL;
> + read_lock_bh(&idev->lock);
> + for (pmc=idev->mc_list; pmc; pmc=pmc->next) {
> + spin_lock_bh(&pmc->mca_lock);
> + if (pmc->mca_sfcount[MCAST_EXCLUDE])
> + type = MLD2_CHANGE_TO_EXCLUDE;
> + else
> + type = MLD2_CHANGE_TO_INCLUDE;
> + skb = add_grec(skb, pmc, type, 0, 0, 1);
> + spin_unlock_bh(&pmc->mca_lock);
> + }
> + read_unlock_bh(&idev->lock);
> + if (skb)
> + mld_sendpack(skb);
> +}
> +
> static void mld_send_report(struct inet6_dev *idev, struct ifmcaddr6 *pmc)
> {
> struct sk_buff *skb = NULL;
> @@ -1617,7 +1639,7 @@ static void mld_send_report(struct inet6_dev *idev, struct ifmcaddr6 *pmc)
> type = MLD2_MODE_IS_EXCLUDE;
> else
> type = MLD2_MODE_IS_INCLUDE;
> - skb = add_grec(skb, pmc, type, 0, 0);
> + skb = add_grec(skb, pmc, type, 0, 0, 0);
> spin_unlock_bh(&pmc->mca_lock);
> }
> } else {
> @@ -1626,7 +1648,7 @@ static void mld_send_report(struct inet6_dev *idev, struct ifmcaddr6 *pmc)
> type = MLD2_MODE_IS_EXCLUDE;
> else
> type = MLD2_MODE_IS_INCLUDE;
> - skb = add_grec(skb, pmc, type, 0, 0);
> + skb = add_grec(skb, pmc, type, 0, 0, 0);
> spin_unlock_bh(&pmc->mca_lock);
> }
> read_unlock_bh(&idev->lock);
> @@ -1671,13 +1693,13 @@ static void mld_send_cr(struct inet6_dev *idev)
> if (pmc->mca_sfmode == MCAST_INCLUDE) {
> type = MLD2_BLOCK_OLD_SOURCES;
> dtype = MLD2_BLOCK_OLD_SOURCES;
> - skb = add_grec(skb, pmc, type, 1, 0);
> - skb = add_grec(skb, pmc, dtype, 1, 1);
> + skb = add_grec(skb, pmc, type, 1, 0, 0);
> + skb = add_grec(skb, pmc, dtype, 1, 1, 0);
> }
> if (pmc->mca_crcount) {
> if (pmc->mca_sfmode == MCAST_EXCLUDE) {
> type = MLD2_CHANGE_TO_INCLUDE;
> - skb = add_grec(skb, pmc, type, 1, 0);
> + skb = add_grec(skb, pmc, type, 1, 0, 0);
> }
> pmc->mca_crcount--;
> if (pmc->mca_crcount == 0) {
> @@ -1708,8 +1730,8 @@ static void mld_send_cr(struct inet6_dev *idev)
> type = MLD2_ALLOW_NEW_SOURCES;
> dtype = MLD2_BLOCK_OLD_SOURCES;
> }
> - skb = add_grec(skb, pmc, type, 0, 0);
> - skb = add_grec(skb, pmc, dtype, 0, 1); /* deleted sources */
> + skb = add_grec(skb, pmc, type, 0, 0, 0);
> + skb = add_grec(skb, pmc, dtype, 0, 1, 0); /* deleted sources */
>
> /* filter mode changes */
> if (pmc->mca_crcount) {
> @@ -1717,7 +1739,7 @@ static void mld_send_cr(struct inet6_dev *idev)
> type = MLD2_CHANGE_TO_EXCLUDE;
> else
> type = MLD2_CHANGE_TO_INCLUDE;
> - skb = add_grec(skb, pmc, type, 0, 0);
> + skb = add_grec(skb, pmc, type, 0, 0, 0);
> pmc->mca_crcount--;
> }
> spin_unlock_bh(&pmc->mca_lock);
> @@ -1825,27 +1847,19 @@ err_out:
> goto out;
> }
>
> -static void mld_resend_report(struct inet6_dev *idev)
> +static void mld_resend_cr(struct inet6_dev *idev)
> {
> - if (MLD_V1_SEEN(idev)) {
> - struct ifmcaddr6 *mcaddr;
> - read_lock_bh(&idev->lock);
> - for (mcaddr = idev->mc_list; mcaddr; mcaddr = mcaddr->next) {
> - if (!(mcaddr->mca_flags & MAF_NOREPORT))
> - igmp6_send(&mcaddr->mca_addr, idev->dev,
> - ICMPV6_MGM_REPORT);
> - }
> - read_unlock_bh(&idev->lock);
> - } else {
> - mld_send_report(idev, NULL);
> - }
> + if (MLD_V1_SEEN(idev))
> + return;
$ git grep -n MLD_V1_SEEN
$
I believe I have removed that macro some time ago. ;)
I presume you mean rather: mld_in_v1_mode(idev)
> + mld_send_initial_cr(idev);
> }
>
> void ipv6_mc_dad_complete(struct inet6_dev *idev)
> {
> idev->mc_dad_count = idev->mc_qrv;
> if (idev->mc_dad_count) {
> - mld_resend_report(idev);
> + mld_resend_cr(idev);
> idev->mc_dad_count--;
> if (idev->mc_dad_count)
> mld_dad_start_timer(idev, idev->mc_maxdelay);
> @@ -1856,7 +1870,7 @@ static void mld_dad_timer_expire(unsigned long data)
> {
> struct inet6_dev *idev = (struct inet6_dev *)data;
>
> - mld_resend_report(idev);
> + mld_resend_cr(idev);
> if (idev->mc_dad_count) {
> idev->mc_dad_count--;
> if (idev->mc_dad_count)
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net-next] ipv6: send Change Status Report after DAD is completed
2014-01-16 11:21 ` Daniel Borkmann
@ 2014-01-16 12:21 ` Flavio Leitner
2014-01-16 12:24 ` Hannes Frederic Sowa
0 siblings, 1 reply; 4+ messages in thread
From: Flavio Leitner @ 2014-01-16 12:21 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: netdev, Hideaki YOSHIFUJI, Hannes Frederic Sowa
On Thu, Jan 16, 2014 at 12:21:30PM +0100, Daniel Borkmann wrote:
> On 01/15/2014 08:10 PM, Flavio Leitner wrote:
> >The RFC 3810 defines two type of messages for multicast
> >listeners. The "Current State Report" message, as the name
> >implies, refreshes the *current* state to the querier.
> >Since the querier sends Query messages periodically, there
> >is no need to retransmit the report.
> >
> >On the other hand, any change should be reported immediately
> >using "State Change Report" messages. Since it's an event
> >triggered by a change and that it can be affected by packet
> >loss, the rfc states it should be retransmitted [RobVar] times
> >to make sure routers will receive timely.
> >
> >Currently, we are sending "Current State Reports" after
> >DAD is completed. Before that, we send messages using
> >unspecified address (::) which should be silently discarded
> >by routers.
> >
> >This patch changes to send "State Change Report" messages
> >after DAD is completed fixing the behavior to be RFC compliant
> >and also to pass TAHI IPv6 testsuite.
> >
> >Signed-off-by: Flavio Leitner <fbl@redhat.com>
> >---
> > net/ipv6/mcast.c | 64 ++++++++++++++++++++++++++++++++++----------------------
> > 1 file changed, 39 insertions(+), 25 deletions(-)
> >
> >diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
> >index 99cd65c..8ac17f5 100644
> >--- a/net/ipv6/mcast.c
> >+++ b/net/ipv6/mcast.c
> >@@ -1493,7 +1493,7 @@ static struct sk_buff *add_grhead(struct sk_buff *skb, struct ifmcaddr6 *pmc,
> > skb_tailroom(skb)) : 0)
> >
> > static struct sk_buff *add_grec(struct sk_buff *skb, struct ifmcaddr6 *pmc,
> >- int type, int gdeleted, int sdeleted)
> >+ int type, int gdeleted, int sdeleted, int crsend)
>
> Maybe we should convert the last three to 'bool' at some point in time?
I'd say so.
> Nit: argument indent
Ok, will fix in the v2
[...]
> >+ if (MLD_V1_SEEN(idev))
> >+ return;
>
> $ git grep -n MLD_V1_SEEN
> $
>
> I believe I have removed that macro some time ago. ;)
>
> I presume you mean rather: mld_in_v1_mode(idev)
Oops, branched from the wrong (old) point here.
I will send a v2.
Thanks,
fbl
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next] ipv6: send Change Status Report after DAD is completed
2014-01-16 12:21 ` Flavio Leitner
@ 2014-01-16 12:24 ` Hannes Frederic Sowa
0 siblings, 0 replies; 4+ messages in thread
From: Hannes Frederic Sowa @ 2014-01-16 12:24 UTC (permalink / raw)
To: Daniel Borkmann, netdev, Hideaki YOSHIFUJI
On Thu, Jan 16, 2014 at 10:21:54AM -0200, Flavio Leitner wrote:
> > >diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
> > >index 99cd65c..8ac17f5 100644
> > >--- a/net/ipv6/mcast.c
> > >+++ b/net/ipv6/mcast.c
> > >@@ -1493,7 +1493,7 @@ static struct sk_buff *add_grhead(struct sk_buff *skb, struct ifmcaddr6 *pmc,
> > > skb_tailroom(skb)) : 0)
> > >
> > > static struct sk_buff *add_grec(struct sk_buff *skb, struct ifmcaddr6 *pmc,
> > >- int type, int gdeleted, int sdeleted)
> > >+ int type, int gdeleted, int sdeleted, int crsend)
> >
> > Maybe we should convert the last three to 'bool' at some point in time?
>
> I'd say so.
Maybe it works out those can be one enum with a specified operation and no
bool vector?
Maybe you could have a look?
Thanks,
Hannes
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-01-16 12:24 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-15 19:10 [PATCH net-next] ipv6: send Change Status Report after DAD is completed Flavio Leitner
2014-01-16 11:21 ` Daniel Borkmann
2014-01-16 12:21 ` Flavio Leitner
2014-01-16 12:24 ` Hannes Frederic Sowa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).