tipc-discussion — User questions & problem reports, project news, developer discussion

Re: [tipc-discussion] [net] tipc: fix link established but not in session

[David M. <da...@da...>]

From: Tuong Lien <tuo...@de...>
Date: Tue, 16 Apr 2019 10:48:07 +0700

> According to the link FSM, when a link endpoint got RESET_MSG (- a
> traditional one without the stopping bit) from its peer, it moves to
> PEER_RESET state and raises a LINK_DOWN event which then resets the
> link itself. Its state will become ESTABLISHING after the reset event
> and the link will be re-established soon after this endpoint starts to
> send ACTIVATE_MSG to the peer.
> 
> There is no problem with this mechanism, however the link resetting has
> cleared the link 'in_session' flag (along with the other important link
> data such as: the link 'mtu') that was correctly set up at the 1st step
> (i.e. when this endpoint received the peer RESET_MSG). As a result, the
> link will become ESTABLISHED, but the 'in_session' flag is not set, and
> all STATE_MSG from its peer will be dropped at the link_validate_msg().
> It means the link not synced and will sooner or later face a failure.
> 
> Since the link reset action is obviously needed for a new link session
> (this is also true in the other situations), the problem here is that
> the link is re-established a bit too early when the link endpoints are
> not really in-sync yet. The commit forces a resync as already done in
> the previous commit 91986ee166cf ("tipc: fix link session and
> re-establish issues") by simply varying the link 'peer_session' value
> at the link_reset().
> 
> Acked-by: Jon Maloy <jon...@er...>
> Signed-off-by: Tuong Lien <tuo...@de...>

Applied.

Re: [tipc-discussion] help with tipc command

[Jon M. <jon...@er...>]

Hi,
I tried it in my version (5.0) , and the parameter was accepted both as 1.1.0 and as 1001000, but the links didn't come up.
It is possible that we have broken something, as this parameter is regarded as obsolete, and not really anything we pay attention to during testing.
But of course it should work, just as tipc-config should continue working without any limitations.
I'll look into this later today, when I find some time.

Still, my first question is if you really need this setting. It is only needed when you are planning to have nodes interconnected on the same LAN/VLAN, but you still want to keep them isolated. Is that so?
And, if you need this isolation, you could use the network id (nowadays called cluster id)  instead.
Just give it a try, while I start some troubleshooting on my part.

BR
///jon



> -----Original Message-----
> From: Rune Torgersen <ru...@in...>
> Sent: 16-Apr-19 12:39
> To: Jon Maloy <jon...@er...>; tipc-
> dis...@li...
> Subject: RE: help with tipc command
> 
> Did find that site after a while.
> 
> However it only shows enable without the domain param.
> 
> I got "tipc bearer enable priority 5 media eth device eth2" to work but still get
> that error on " tipc bearer enable domain 1.1.0 priority 5 media eth device
> eth2"
> 
> Nowhere in the documentation does it show me the correct format for the
> domain param.
> 
> -----Original Message-----
> From: Jon Maloy <jon...@er...>
> Sent: Tuesday, April 16, 2019 11:16
> To: Rune Torgersen <ru...@in...>; tipc-
> dis...@li...
> Subject: RE: help with tipc command
> 
> Hi Rune,
> Have a look here:
> 
> http://tipc.io/getting_started.html
> 
> Hilsen
> ///jon
> 
> 
> > -----Original Message-----
> > From: Rune Torgersen <ru...@in...>
> > Sent: 16-Apr-19 11:41
> > To: tip...@li...
> > Subject: [tipc-discussion] help with tipc command
> >
> > I am trying to convert some scripts from using tipc-config to newer
> > tipc command  (no, not "ip tipc" yet as it is not available on Ubuntu 16.04).
> >
> > I was not planning g on doing this unit we moved to Ubuntu 18, but I
> > just noticed that with the lastest ubuntu 16.04 kernel (4.4.0-145)
> > tipc-config -b no longer works (worked on 4.4.0-143).
> >
> > I manager to figure out syntax for bearer disable, but have problems
> > with bearer enable.
> >
> >
> > Old command was
> > tipc-config -be=eth:eth2/1.1.0/5
> >
> > New is
> >
> > But gets an error
> > root@test10020u3:~# tipc bearer enable domain 1.1.0 priority 5 media
> > eth device eth2
> > error: Invalid argument
> > root@test10020u3:~# dmesg -c
> > [ 2896.786648] Bearer <eth:eth2> rejected, illegal discovery domain
> >
> >
> > Also, what would be the equivalents of:
> > tipc-config -netid=$netid
> > tipc-config -a=1.1.${unit}
> >
> >
> >
> >
> > _______________________________________________
> > tipc-discussion mailing list
> > tip...@li...
> > https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] help with tipc command

[Rune T. <ru...@in...>]

Did find that site after a while.

However it only shows enable without the domain param.

I got "tipc bearer enable priority 5 media eth device eth2" to work but still get that error on " tipc bearer enable domain 1.1.0 priority 5 media eth device eth2"

Nowhere in the documentation does it show me the correct format for the domain param.

-----Original Message-----
From: Jon Maloy <jon...@er...> 
Sent: Tuesday, April 16, 2019 11:16
To: Rune Torgersen <ru...@in...>; tip...@li...
Subject: RE: help with tipc command

Hi Rune,
Have a look here:

http://tipc.io/getting_started.html

Hilsen
///jon


> -----Original Message-----
> From: Rune Torgersen <ru...@in...>
> Sent: 16-Apr-19 11:41
> To: tip...@li...
> Subject: [tipc-discussion] help with tipc command
> 
> I am trying to convert some scripts from using tipc-config to newer tipc
> command  (no, not "ip tipc" yet as it is not available on Ubuntu 16.04).
> 
> I was not planning g on doing this unit we moved to Ubuntu 18, but I just
> noticed that with the lastest ubuntu 16.04 kernel (4.4.0-145) tipc-config -b no
> longer works (worked on 4.4.0-143).
> 
> I manager to figure out syntax for bearer disable, but have problems with
> bearer enable.
> 
> 
> Old command was
> tipc-config -be=eth:eth2/1.1.0/5
> 
> New is
> 
> But gets an error
> root@test10020u3:~# tipc bearer enable domain 1.1.0 priority 5 media eth
> device eth2
> error: Invalid argument
> root@test10020u3:~# dmesg -c
> [ 2896.786648] Bearer <eth:eth2> rejected, illegal discovery domain
> 
> 
> Also, what would be the equivalents of:
> tipc-config -netid=$netid
> tipc-config -a=1.1.${unit}
> 
> 
> 
> 
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] help with tipc command

[Jon M. <jon...@er...>]

Hi Rune,
Have a look here:

http://tipc.io/getting_started.html

Hilsen
///jon


> -----Original Message-----
> From: Rune Torgersen <ru...@in...>
> Sent: 16-Apr-19 11:41
> To: tip...@li...
> Subject: [tipc-discussion] help with tipc command
> 
> I am trying to convert some scripts from using tipc-config to newer tipc
> command  (no, not "ip tipc" yet as it is not available on Ubuntu 16.04).
> 
> I was not planning g on doing this unit we moved to Ubuntu 18, but I just
> noticed that with the lastest ubuntu 16.04 kernel (4.4.0-145) tipc-config -b no
> longer works (worked on 4.4.0-143).
> 
> I manager to figure out syntax for bearer disable, but have problems with
> bearer enable.
> 
> 
> Old command was
> tipc-config -be=eth:eth2/1.1.0/5
> 
> New is
> 
> But gets an error
> root@test10020u3:~# tipc bearer enable domain 1.1.0 priority 5 media eth
> device eth2
> error: Invalid argument
> root@test10020u3:~# dmesg -c
> [ 2896.786648] Bearer <eth:eth2> rejected, illegal discovery domain
> 
> 
> Also, what would be the equivalents of:
> tipc-config -netid=$netid
> tipc-config -a=1.1.${unit}
> 
> 
> 
> 
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] help with tipc command

[Rune T. <ru...@in...>]

I am trying to convert some scripts from using tipc-config to newer tipc command  (no, not "ip tipc" yet as it is not available on Ubuntu 16.04).

I was not planning g on doing this unit we moved to Ubuntu 18, but I just noticed that with the lastest ubuntu 16.04 kernel (4.4.0-145) tipc-config -b no longer works (worked on 4.4.0-143).

I manager to figure out syntax for bearer disable, but have problems with bearer enable.


Old command was
tipc-config -be=eth:eth2/1.1.0/5

New is

But gets an error
root@test10020u3:~# tipc bearer enable domain 1.1.0 priority 5 media eth device eth2
error: Invalid argument
root@test10020u3:~# dmesg -c
[ 2896.786648] Bearer <eth:eth2> rejected, illegal discovery domain


Also, what would be the equivalents of:
tipc-config -netid=$netid
tipc-config -a=1.1.${unit}

[tipc-discussion] [net] tipc: fix link established but not in session

[Tuong L. <tuo...@de...>]

According to the link FSM, when a link endpoint got RESET_MSG (- a
traditional one without the stopping bit) from its peer, it moves to
PEER_RESET state and raises a LINK_DOWN event which then resets the
link itself. Its state will become ESTABLISHING after the reset event
and the link will be re-established soon after this endpoint starts to
send ACTIVATE_MSG to the peer.

There is no problem with this mechanism, however the link resetting has
cleared the link 'in_session' flag (along with the other important link
data such as: the link 'mtu') that was correctly set up at the 1st step
(i.e. when this endpoint received the peer RESET_MSG). As a result, the
link will become ESTABLISHED, but the 'in_session' flag is not set, and
all STATE_MSG from its peer will be dropped at the link_validate_msg().
It means the link not synced and will sooner or later face a failure.

Since the link reset action is obviously needed for a new link session
(this is also true in the other situations), the problem here is that
the link is re-established a bit too early when the link endpoints are
not really in-sync yet. The commit forces a resync as already done in
the previous commit 91986ee166cf ("tipc: fix link session and
re-establish issues") by simply varying the link 'peer_session' value
at the link_reset().

Acked-by: Jon Maloy <jon...@er...>
Signed-off-by: Tuong Lien <tuo...@de...>
---
 net/tipc/link.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index 3cb9f326ee6f..6053489c8063 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -876,6 +876,8 @@ void tipc_link_reset(struct tipc_link *l)
 	__skb_queue_head_init(&list);
 
 	l->in_session = false;
+	/* Force re-synch of peer session number before establishing */
+	l->peer_session--;
 	l->session++;
 	l->mtu = l->advertised_mtu;
 
-- 
2.13.7

Re: [tipc-discussion] [net] tipc: missing entries in name table of publications

[Jon M. <jon...@er...>]

Hi Hoang,
Yes, but the code for collecting data in the stack was different with "native" tipc-config vs. now, when tipc-config in reality is using the same code as "tipc" just via the "compat" layer.
Not sure when this changed was done, but probably at the uplift to sp2.

///jon


> -----Original Message-----
> From: Hoang Le <hoa...@de...>
> Sent: 9-Apr-19 04:14
> To: Jon Maloy <jon...@er...>; ma...@do...;
> yin...@wi...; tip...@li...
> Subject: RE: [net] tipc: missing entries in name table of publications
> 
> Hi Jon,
> 
> In my reproduction, I'm seeing the problem either tipc-config.
> I guest almost systems were not using as special service type as my test, then
> all node state service type fit in one skb.
> 
> Regards,
> Hoang
> -----Original Message-----
> From: Jon Maloy <jon...@er...>
> Sent: Tuesday, April 9, 2019 5:39 AM
> To: Hoang Huu Le <hoa...@de...>; ma...@do...;
> yin...@wi...; tip...@li...
> Subject: RE: [net] tipc: missing entries in name table of publications
> 
> Nice job. I am just flabbergasted by the fact that this seems to be a bug that
> has always been around, and still it wasn't seen until now, with all the
> systems with huge name tables we have running around in the world.
> Maybe most people are still using "tipc-config"....
> 
> Acked-by: Jon Maloy <jon...@er...>
> 
> 
> > -----Original Message-----
> > From: Hoang Le <hoa...@de...>
> > Sent: 8-Apr-19 07:08
> > To: Jon Maloy <jon...@er...>; ma...@do...;
> > yin...@wi...; tip...@li...
> > Subject: [net] tipc: missing entries in name table of publications
> >
> > When binding multiple services with specific type 1Ki, 2Ki.., this
> > leads to some entries in the name table of publications missing when
> > listed out via 'tipc name show'.
> >
> > The problem is at identify zero last_type conditional provided via
> > netlink. The first is initial 'type' when starting name table
> > dummping. The second is continuously with zero type (node state
> > service type). Then, lookup function failure to finding node state service
> type next iteration.
> >
> > To solve this, adding more conditional to marked as dirty type and
> > lookup correct service type for the next iteration instead of select
> > the first service as initial 'type' zero.
> >
> > Signed-off-by: Hoang Le <hoa...@de...>
> > ---
> >  net/tipc/name_table.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c index
> > bff241f03525..89993afe0fbd 100644
> > --- a/net/tipc/name_table.c
> > +++ b/net/tipc/name_table.c
> > @@ -909,7 +909,8 @@ static int tipc_nl_service_list(struct net *net,
> > struct tipc_nl_msg *msg,
> >  	for (; i < TIPC_NAMETBL_SIZE; i++) {
> >  		head = &tn->nametbl->services[i];
> >
> > -		if (*last_type) {
> > +		if (*last_type ||
> > +		    (!i && *last_key && (*last_lower == *last_key))) {
> >  			service = tipc_service_find(net, *last_type);
> >  			if (!service)
> >  				return -EPIPE;
> > --
> > 2.17.1
> 

Re: [tipc-discussion] [net-next v2 1/1] tipc: use standard write_lock & unlock functions when creating node

[David M. <da...@da...>]

From: Jon Maloy <jon...@er...>
Date: Thu, 11 Apr 2019 21:56:28 +0200

> In the function tipc_node_create() we protect the peer capability field
> by using the node rw_lock. However, we access the lock directly instead
> of using the dedicated functions for this, as we do everywhere else in
> node.c. This cosmetic spot is fixed here.
> 
> Fixes: 40999f11ce67 ("tipc: make link capability update thread safe")
> Signed-off-by: Jon Maloy <jon...@er...>
> 
> ---
> v2: corrected format of "Fixed:" tag

Applied, thanks Jon.

[tipc-discussion] [net-next v2 1/1] tipc: use standard write_lock & unlock functions when creating node

[Jon M. <jon...@er...>]

In the function tipc_node_create() we protect the peer capability field
by using the node rw_lock. However, we access the lock directly instead
of using the dedicated functions for this, as we do everywhere else in
node.c. This cosmetic spot is fixed here.

Fixes: 40999f11ce67 ("tipc: make link capability update thread safe")
Signed-off-by: Jon Maloy <jon...@er...>

---
v2: corrected format of "Fixed:" tag
---
 net/tipc/node.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/tipc/node.c b/net/tipc/node.c
index 3469b5d..7478e2d 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -375,14 +375,15 @@ static struct tipc_node *tipc_node_create(struct net *net, u32 addr,
 		if (n->capabilities == capabilities)
 			goto exit;
 		/* Same node may come back with new capabilities */
-		write_lock_bh(&n->lock);
+		tipc_node_write_lock(n);
 		n->capabilities = capabilities;
 		for (bearer_id = 0; bearer_id < MAX_BEARERS; bearer_id++) {
 			l = n->links[bearer_id].link;
 			if (l)
 				tipc_link_update_caps(l, capabilities);
 		}
-		write_unlock_bh(&n->lock);
+		tipc_node_write_unlock_fast(n);
+
 		/* Calculate cluster capabilities */
 		tn->capabilities = TIPC_NODE_CAPABILITIES;
 		list_for_each_entry_rcu(temp_node, &tn->node_list, list) {
-- 
2.1.4

Re: [tipc-discussion] [net-next 1/1] tipc: use standard write_lock & unlock functions when creating node

[David M. <da...@da...>]

From: Jon Maloy <jon...@er...>
Date: Thu, 11 Apr 2019 20:27:08 +0200

> In the function tipc_node_create() we protect the peer capability field
> by using the node rw_lock. However, we access the lock directly instead
> of using the dedicated functions for this, as we do everywhere else in
> node.c. This cosmetic spot is fixed here.
> 
> Fixes: 40999f11ce67 tipc: make link capability update thread safe
> Signed-off-by: Jon Maloy <jon...@er...>

Hello Jon, please format Fixes: tags as follows:

Fixes: SHA_ID_12DIGITS ("Commit header line text")

Thank you.

[tipc-discussion] [net-next 1/1] tipc: use standard write_lock & unlock functions when creating node

[Jon M. <jon...@er...>]

In the function tipc_node_create() we protect the peer capability field
by using the node rw_lock. However, we access the lock directly instead
of using the dedicated functions for this, as we do everywhere else in
node.c. This cosmetic spot is fixed here.

Fixes: 40999f11ce67 tipc: make link capability update thread safe
Signed-off-by: Jon Maloy <jon...@er...>
---
 net/tipc/node.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/tipc/node.c b/net/tipc/node.c
index 3469b5d..7478e2d 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -375,14 +375,15 @@ static struct tipc_node *tipc_node_create(struct net *net, u32 addr,
 		if (n->capabilities == capabilities)
 			goto exit;
 		/* Same node may come back with new capabilities */
-		write_lock_bh(&n->lock);
+		tipc_node_write_lock(n);
 		n->capabilities = capabilities;
 		for (bearer_id = 0; bearer_id < MAX_BEARERS; bearer_id++) {
 			l = n->links[bearer_id].link;
 			if (l)
 				tipc_link_update_caps(l, capabilities);
 		}
-		write_unlock_bh(&n->lock);
+		tipc_node_write_unlock_fast(n);
+
 		/* Calculate cluster capabilities */
 		tn->capabilities = TIPC_NODE_CAPABILITIES;
 		list_for_each_entry_rcu(temp_node, &tn->node_list, list) {
-- 
2.1.4

Re: [tipc-discussion] [net] tipc: missing entries in name table of publications

[David M. <da...@da...>]

From: Hoang Le <hoa...@de...>
Date: Tue,  9 Apr 2019 14:59:24 +0700

> When binding multiple services with specific type 1Ki, 2Ki..,
> this leads to some entries in the name table of publications
> missing when listed out via 'tipc name show'.
> 
> The problem is at identify zero last_type conditional provided
> via netlink. The first is initial 'type' when starting name table
> dummping. The second is continuously with zero type (node state
> service type). Then, lookup function failure to finding node state
> service type in next iteration.
> 
> To solve this, adding more conditional to marked as dirty type and
> lookup correct service type for the next iteration instead of select
> the first service as initial 'type' zero.
> 
> Acked-by: Jon Maloy <jon...@er...>
> Signed-off-by: Hoang Le <hoa...@de...>

Applied, and queued up for -stable.

Please provide an appropriate Fixes: tag next time.

Thank you.

Re: [tipc-discussion] [net] tipc: missing entries in name table of publications

[Hoang L. <hoa...@de...>]

Hi Jon,

In my reproduction, I'm seeing the problem either tipc-config. 
I guest almost systems were not using as special service type as my test, then all node state service type fit in one skb. 

Regards,
Hoang
-----Original Message-----
From: Jon Maloy <jon...@er...> 
Sent: Tuesday, April 9, 2019 5:39 AM
To: Hoang Huu Le <hoa...@de...>; ma...@do...; yin...@wi...; tip...@li...
Subject: RE: [net] tipc: missing entries in name table of publications

Nice job. I am just flabbergasted by the fact that this seems to be a bug that has always been around, and still it wasn't seen
until now, with all the systems with huge name tables we have running around in the world.
Maybe most people are still using "tipc-config"....

Acked-by: Jon Maloy <jon...@er...>


> -----Original Message-----
> From: Hoang Le <hoa...@de...>
> Sent: 8-Apr-19 07:08
> To: Jon Maloy <jon...@er...>; ma...@do...;
> yin...@wi...; tip...@li...
> Subject: [net] tipc: missing entries in name table of publications
> 
> When binding multiple services with specific type 1Ki, 2Ki.., this leads to some
> entries in the name table of publications missing when listed out via 'tipc
> name show'.
> 
> The problem is at identify zero last_type conditional provided via netlink. The
> first is initial 'type' when starting name table dummping. The second is
> continuously with zero type (node state service type). Then, lookup function
> failure to finding node state service type next iteration.
> 
> To solve this, adding more conditional to marked as dirty type and lookup
> correct service type for the next iteration instead of select the first service as
> initial 'type' zero.
> 
> Signed-off-by: Hoang Le <hoa...@de...>
> ---
>  net/tipc/name_table.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c index
> bff241f03525..89993afe0fbd 100644
> --- a/net/tipc/name_table.c
> +++ b/net/tipc/name_table.c
> @@ -909,7 +909,8 @@ static int tipc_nl_service_list(struct net *net, struct
> tipc_nl_msg *msg,
>  	for (; i < TIPC_NAMETBL_SIZE; i++) {
>  		head = &tn->nametbl->services[i];
> 
> -		if (*last_type) {
> +		if (*last_type ||
> +		    (!i && *last_key && (*last_lower == *last_key))) {
>  			service = tipc_service_find(net, *last_type);
>  			if (!service)
>  				return -EPIPE;
> --
> 2.17.1

[tipc-discussion] [net] tipc: missing entries in name table of publications

[Hoang Le <hoa...@de...>]

When binding multiple services with specific type 1Ki, 2Ki..,
this leads to some entries in the name table of publications
missing when listed out via 'tipc name show'.

The problem is at identify zero last_type conditional provided
via netlink. The first is initial 'type' when starting name table
dummping. The second is continuously with zero type (node state
service type). Then, lookup function failure to finding node state
service type in next iteration.

To solve this, adding more conditional to marked as dirty type and
lookup correct service type for the next iteration instead of select
the first service as initial 'type' zero.

Acked-by: Jon Maloy <jon...@er...>
Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/name_table.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
index bff241f03525..89993afe0fbd 100644
--- a/net/tipc/name_table.c
+++ b/net/tipc/name_table.c
@@ -909,7 +909,8 @@ static int tipc_nl_service_list(struct net *net, struct tipc_nl_msg *msg,
 	for (; i < TIPC_NAMETBL_SIZE; i++) {
 		head = &tn->nametbl->services[i];
 
-		if (*last_type) {
+		if (*last_type ||
+		    (!i && *last_key && (*last_lower == *last_key))) {
 			service = tipc_service_find(net, *last_type);
 			if (!service)
 				return -EPIPE;
-- 
2.17.1

[tipc-discussion] [net] tipc: fix link established but not in session

[Tuong L. <tuo...@de...>]

According to the link FSM, when a link endpoint got RESET_MSG (- a
traditional one without the stopping bit) from its peer, it moves to
PEER_RESET state and raises a LINK_DOWN event which then resets the
link itself. Its state will become ESTABLISHING after the reset event
and the link will be re-established soon after this endpoint starts to
send ACTIVATE_MSG to the peer.

There is no problem with this mechanism, however the link resetting has
cleared the link 'in_session' flag (along with the other important link
data such as: the link 'mtu') that was correctly set up at the 1st step
(i.e. when this endpoint received the peer RESET_MSG). As a result, the
link will become ESTABLISHED, but the 'in_session' flag is not set, and
all STATE_MSG from its peer will be dropped at the link_validate_msg().
It means the link not synced and will sooner or later face a failure.

Since the link reset action is obviously needed for a new link session
(this is also true in the other situations), the problem here is that
the link is re-established a bit too early when the link endpoints are
not really in-sync yet. The commit forces a resync as already done in
the previous commit 91986ee166cf ("tipc: fix link session and
re-establish issues") by simply varying the link 'peer_session' value
at the link_reset().

Signed-off-by: Tuong Lien <tuo...@de...>
---
 net/tipc/link.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index 3cb9f326ee6f..6053489c8063 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -876,6 +876,8 @@ void tipc_link_reset(struct tipc_link *l)
 	__skb_queue_head_init(&list);
 
 	l->in_session = false;
+	/* Force re-synch of peer session number before establishing */
+	l->peer_session--;
 	l->session++;
 	l->mtu = l->advertised_mtu;
 
-- 
2.13.7

Re: [tipc-discussion] [net] tipc: missing entries in name table of publications

[Jon M. <jon...@er...>]

Nice job. I am just flabbergasted by the fact that this seems to be a bug that has always been around, and still it wasn't seen until now, with all the systems with huge name tables we have running around in the world.
Maybe most people are still using "tipc-config"....

Acked-by: Jon Maloy <jon...@er...>


> -----Original Message-----
> From: Hoang Le <hoa...@de...>
> Sent: 8-Apr-19 07:08
> To: Jon Maloy <jon...@er...>; ma...@do...;
> yin...@wi...; tip...@li...
> Subject: [net] tipc: missing entries in name table of publications
> 
> When binding multiple services with specific type 1Ki, 2Ki.., this leads to some
> entries in the name table of publications missing when listed out via 'tipc
> name show'.
> 
> The problem is at identify zero last_type conditional provided via netlink. The
> first is initial 'type' when starting name table dummping. The second is
> continuously with zero type (node state service type). Then, lookup function
> failure to finding node state service type next iteration.
> 
> To solve this, adding more conditional to marked as dirty type and lookup
> correct service type for the next iteration instead of select the first service as
> initial 'type' zero.
> 
> Signed-off-by: Hoang Le <hoa...@de...>
> ---
>  net/tipc/name_table.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c index
> bff241f03525..89993afe0fbd 100644
> --- a/net/tipc/name_table.c
> +++ b/net/tipc/name_table.c
> @@ -909,7 +909,8 @@ static int tipc_nl_service_list(struct net *net, struct
> tipc_nl_msg *msg,
>  	for (; i < TIPC_NAMETBL_SIZE; i++) {
>  		head = &tn->nametbl->services[i];
> 
> -		if (*last_type) {
> +		if (*last_type ||
> +		    (!i && *last_key && (*last_lower == *last_key))) {
>  			service = tipc_service_find(net, *last_type);
>  			if (!service)
>  				return -EPIPE;
> --
> 2.17.1

[tipc-discussion] [net] tipc: missing entries in name table of publications

[Hoang Le <hoa...@de...>]

When binding multiple services with specific type 1Ki, 2Ki..,
this leads to some entries in the name table of publications
missing when listed out via 'tipc name show'.

The problem is at identify zero last_type conditional provided
via netlink. The first is initial 'type' when starting name table
dummping. The second is continuously with zero type (node state
service type). Then, lookup function failure to finding node state
service type next iteration.

To solve this, adding more conditional to marked as dirty type and
lookup correct service type for the next iteration instead of select
the first service as initial 'type' zero.

Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/name_table.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
index bff241f03525..89993afe0fbd 100644
--- a/net/tipc/name_table.c
+++ b/net/tipc/name_table.c
@@ -909,7 +909,8 @@ static int tipc_nl_service_list(struct net *net, struct tipc_nl_msg *msg,
 	for (; i < TIPC_NAMETBL_SIZE; i++) {
 		head = &tn->nametbl->services[i];
 
-		if (*last_type) {
+		if (*last_type ||
+		    (!i && *last_key && (*last_lower == *last_key))) {
 			service = tipc_service_find(net, *last_type);
 			if (!service)
 				return -EPIPE;
-- 
2.17.1

Re: [tipc-discussion] [net-next 0/3] tipc: improve TIPC unicast link throughput

[David M. <da...@da...>]

From: Tuong Lien <tuo...@de...>
Date: Thu,  4 Apr 2019 11:09:50 +0700

> The series introduces an algorithm to improve TIPC throughput especially
> in terms of packet loss, also tries to reduce packet duplication due to
> overactive NACK sending mechanism.
> 
> The link failover situation is also covered by the patches.

Series applied, thanks.

Re: [tipc-discussion] [net-next] tipc: add NULL pointer check

[David M. <da...@da...>]

From: Hoang Le <hoa...@de...>
Date: Wed,  3 Apr 2019 13:05:04 +0700

> skb somehow dequeued out of inputq before processing, it causes to
> NULL pointer and kernel crashed.
> 
> Add checking skb valid before using.
> 
> Fixes: c55c8edafa9 ("tipc: smooth change between replicast and broadcast")
> Reported-by: Tuong Lien Tong <tuo...@de...>
> Acked-by: Ying Xue <yin...@wi...>
> Signed-off-by: Hoang Le <hoa...@de...>

Applied.

[tipc-discussion] [net-next 3/3] tipc: adapt link failover for new Gap-ACK algorithm

[Tuong L. <tuo...@de...>]

In commit 0ae955e2656d ("tipc: improve TIPC throughput by Gap ACK
blocks"), we enhance the link transmq by releasing as many packets as
possible with the multi-ACKs from peer node. This also means the queue
is now non-linear and the peer link deferdq becomes vital.

Whereas, in the case of link failover, all messages in the link transmq
need to be transmitted as tunnel messages in such a way that message
sequentiality and cardinality per sender is preserved. This requires us
to maintain the link deferdq somehow, so that when the tunnel messages
arrive, the inner user messages along with the ones in the deferdq will
be delivered to upper layer correctly.

The commit accomplishes this by defining a new queue in the TIPC link
structure to hold the old link deferdq when link failover happens and
process it upon receipt of tunnel messages.

Also, in the case of link syncing, the link deferdq will not be purged
to avoid unnecessary retransmissions that in the worst case will fail
because the packets might have been freed on the sending side.

Acked-by: Ying Xue <yin...@wi...>
Acked-by: Jon Maloy <jon...@er...>
Signed-off-by: Tuong Lien <tuo...@de...>
---
 net/tipc/link.c | 106 ++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 80 insertions(+), 26 deletions(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index 1f2cde0d025f..3cb9f326ee6f 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -151,6 +151,7 @@ struct tipc_link {
 	/* Failover/synch */
 	u16 drop_point;
 	struct sk_buff *failover_reasm_skb;
+	struct sk_buff_head failover_deferdq;
 
 	/* Max packet negotiation */
 	u16 mtu;
@@ -498,6 +499,7 @@ bool tipc_link_create(struct net *net, char *if_name, int bearer_id,
 	__skb_queue_head_init(&l->transmq);
 	__skb_queue_head_init(&l->backlogq);
 	__skb_queue_head_init(&l->deferdq);
+	__skb_queue_head_init(&l->failover_deferdq);
 	skb_queue_head_init(&l->wakeupq);
 	skb_queue_head_init(l->inputq);
 	return true;
@@ -888,6 +890,7 @@ void tipc_link_reset(struct tipc_link *l)
 	__skb_queue_purge(&l->transmq);
 	__skb_queue_purge(&l->deferdq);
 	__skb_queue_purge(&l->backlogq);
+	__skb_queue_purge(&l->failover_deferdq);
 	l->backlog[TIPC_LOW_IMPORTANCE].len = 0;
 	l->backlog[TIPC_MEDIUM_IMPORTANCE].len = 0;
 	l->backlog[TIPC_HIGH_IMPORTANCE].len = 0;
@@ -1159,34 +1162,14 @@ static bool tipc_data_input(struct tipc_link *l, struct sk_buff *skb,
  * Consumes buffer
  */
 static int tipc_link_input(struct tipc_link *l, struct sk_buff *skb,
-			   struct sk_buff_head *inputq)
+			   struct sk_buff_head *inputq,
+			   struct sk_buff **reasm_skb)
 {
 	struct tipc_msg *hdr = buf_msg(skb);
-	struct sk_buff **reasm_skb = &l->reasm_buf;
 	struct sk_buff *iskb;
 	struct sk_buff_head tmpq;
 	int usr = msg_user(hdr);
-	int rc = 0;
 	int pos = 0;
-	int ipos = 0;
-
-	if (unlikely(usr == TUNNEL_PROTOCOL)) {
-		if (msg_type(hdr) == SYNCH_MSG) {
-			__skb_queue_purge(&l->deferdq);
-			goto drop;
-		}
-		if (!tipc_msg_extract(skb, &iskb, &ipos))
-			return rc;
-		kfree_skb(skb);
-		skb = iskb;
-		hdr = buf_msg(skb);
-		if (less(msg_seqno(hdr), l->drop_point))
-			goto drop;
-		if (tipc_data_input(l, skb, inputq))
-			return rc;
-		usr = msg_user(hdr);
-		reasm_skb = &l->failover_reasm_skb;
-	}
 
 	if (usr == MSG_BUNDLER) {
 		skb_queue_head_init(&tmpq);
@@ -1211,11 +1194,66 @@ static int tipc_link_input(struct tipc_link *l, struct sk_buff *skb,
 		tipc_link_bc_init_rcv(l->bc_rcvlink, hdr);
 		tipc_bcast_unlock(l->net);
 	}
-drop:
+
 	kfree_skb(skb);
 	return 0;
 }
 
+/* tipc_link_tnl_rcv() - receive TUNNEL_PROTOCOL message, drop or process the
+ *			 inner message along with the ones in the old link's
+ *			 deferdq
+ * @l: tunnel link
+ * @skb: TUNNEL_PROTOCOL message
+ * @inputq: queue to put messages ready for delivery
+ */
+static int tipc_link_tnl_rcv(struct tipc_link *l, struct sk_buff *skb,
+			     struct sk_buff_head *inputq)
+{
+	struct sk_buff **reasm_skb = &l->failover_reasm_skb;
+	struct sk_buff_head *fdefq = &l->failover_deferdq;
+	struct tipc_msg *hdr = buf_msg(skb);
+	struct sk_buff *iskb;
+	int ipos = 0;
+	int rc = 0;
+	u16 seqno;
+
+	/* SYNCH_MSG */
+	if (msg_type(hdr) == SYNCH_MSG)
+		goto drop;
+
+	/* FAILOVER_MSG */
+	if (!tipc_msg_extract(skb, &iskb, &ipos)) {
+		pr_warn_ratelimited("Cannot extract FAILOVER_MSG, defq: %d\n",
+				    skb_queue_len(fdefq));
+		return rc;
+	}
+
+	do {
+		seqno = buf_seqno(iskb);
+
+		if (unlikely(less(seqno, l->drop_point))) {
+			kfree_skb(iskb);
+			continue;
+		}
+
+		if (unlikely(seqno != l->drop_point)) {
+			__tipc_skb_queue_sorted(fdefq, seqno, iskb);
+			continue;
+		}
+
+		l->drop_point++;
+
+		if (!tipc_data_input(l, iskb, inputq))
+			rc |= tipc_link_input(l, iskb, inputq, reasm_skb);
+		if (unlikely(rc))
+			break;
+	} while ((iskb = __tipc_skb_dequeue(fdefq, l->drop_point)));
+
+drop:
+	kfree_skb(skb);
+	return rc;
+}
+
 static bool tipc_link_release_pkts(struct tipc_link *l, u16 acked)
 {
 	bool released = false;
@@ -1457,8 +1495,11 @@ int tipc_link_rcv(struct tipc_link *l, struct sk_buff *skb,
 		/* Deliver packet */
 		l->rcv_nxt++;
 		l->stats.recv_pkts++;
-		if (!tipc_data_input(l, skb, l->inputq))
-			rc |= tipc_link_input(l, skb, l->inputq);
+
+		if (unlikely(msg_user(hdr) == TUNNEL_PROTOCOL))
+			rc |= tipc_link_tnl_rcv(l, skb, l->inputq);
+		else if (!tipc_data_input(l, skb, l->inputq))
+			rc |= tipc_link_input(l, skb, l->inputq, &l->reasm_buf);
 		if (unlikely(++l->rcv_unacked >= TIPC_MIN_LINK_WIN))
 			rc |= tipc_link_build_state_msg(l, xmitq);
 		if (unlikely(rc & ~TIPC_LINK_SND_STATE))
@@ -1588,6 +1629,7 @@ void tipc_link_create_dummy_tnl_msg(struct tipc_link *l,
 void tipc_link_tnl_prepare(struct tipc_link *l, struct tipc_link *tnl,
 			   int mtyp, struct sk_buff_head *xmitq)
 {
+	struct sk_buff_head *fdefq = &tnl->failover_deferdq;
 	struct sk_buff *skb, *tnlskb;
 	struct tipc_msg *hdr, tnlhdr;
 	struct sk_buff_head *queue = &l->transmq;
@@ -1615,7 +1657,11 @@ void tipc_link_tnl_prepare(struct tipc_link *l, struct tipc_link *tnl,
 	/* Initialize reusable tunnel packet header */
 	tipc_msg_init(tipc_own_addr(l->net), &tnlhdr, TUNNEL_PROTOCOL,
 		      mtyp, INT_H_SIZE, l->addr);
-	pktcnt = skb_queue_len(&l->transmq) + skb_queue_len(&l->backlogq);
+	if (mtyp == SYNCH_MSG)
+		pktcnt = l->snd_nxt - buf_seqno(skb_peek(&l->transmq));
+	else
+		pktcnt = skb_queue_len(&l->transmq);
+	pktcnt += skb_queue_len(&l->backlogq);
 	msg_set_msgcnt(&tnlhdr, pktcnt);
 	msg_set_bearer_id(&tnlhdr, l->peer_bearer_id);
 tnl:
@@ -1646,6 +1692,14 @@ void tipc_link_tnl_prepare(struct tipc_link *l, struct tipc_link *tnl,
 		tnl->drop_point = l->rcv_nxt;
 		tnl->failover_reasm_skb = l->reasm_buf;
 		l->reasm_buf = NULL;
+
+		/* Failover the link's deferdq */
+		if (unlikely(!skb_queue_empty(fdefq))) {
+			pr_warn("Link failover deferdq not empty: %d!\n",
+				skb_queue_len(fdefq));
+			__skb_queue_purge(fdefq);
+		}
+		skb_queue_splice_init(&l->deferdq, fdefq);
 	}
 }
 
-- 
2.13.7

[tipc-discussion] [net-next 0/3] tipc: improve TIPC unicast link throughput

[Tuong L. <tuo...@de...>]

The series introduces an algorithm to improve TIPC throughput especially
in terms of packet loss, also tries to reduce packet duplication due to
overactive NACK sending mechanism.

The link failover situation is also covered by the patches.

Tuong Lien (3):
  tipc: improve TIPC throughput by Gap ACK blocks
  tipc: reduce duplicate packets for unicast traffic
  tipc: adapt link failover for new Gap-ACK algorithm

 net/tipc/link.c | 266 ++++++++++++++++++++++++++++++++++++++++++++++----------
 net/tipc/msg.h  |  52 +++++++++++
 net/tipc/node.h |   6 +-
 3 files changed, 276 insertions(+), 48 deletions(-)

-- 
2.13.7

[tipc-discussion] [net-next 2/3] tipc: reduce duplicate packets for unicast traffic

[Tuong L. <tuo...@de...>]

For unicast transmission, the current NACK sending althorithm is over-
active that forces the sending side to retransmit a packet that is not
really lost but just arrived at the receiving side with some delay, or
even retransmit same packets that have already been retransmitted
before. As a result, many duplicates are observed also under normal
condition, ie. without packet loss.

One example case is: node1 transmits 1 2 3 4 10 5 6 7 8 9, when node2
receives packet #10, it puts into the deferdq. When the packet #5 comes
it sends NACK with gap [6 - 9]. However, shortly after that, when
packet #6 arrives, it pulls out packet #10 from the deferfq, but it is
still out of order, so it makes another NACK with gap [7 - 9] and so on
... Finally, node1 has to retransmit the packets 5 6 7 8 9 a number of
times, but in fact all the packets are not lost at all, so duplicates!

This commit reduces duplicates by changing the condition to send NACK,
also restricting the retransmissions on individual packets via a timer
of about 1ms. However, it also needs to say that too tricky condition
for NACKs or too long timeout value for retransmissions will result in
performance reducing! The criterias in this commit are found to be
effective for both the requirements to reduce duplicates but not affect
performance.

The tipc_link_rcv() is also improved to only dequeue skb from the link
deferdq if it is expected (ie. its seqno <= rcv_nxt).

Acked-by: Ying Xue <yin...@wi...>
Acked-by: Jon Maloy <jon...@er...>
Signed-off-by: Tuong Lien <tuo...@de...>
---
 net/tipc/link.c | 26 ++++++++++++++++----------
 net/tipc/msg.h  | 21 +++++++++++++++++++++
 2 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index 5aee1ed23ba9..1f2cde0d025f 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -209,6 +209,7 @@ enum {
 };
 
 #define TIPC_BC_RETR_LIM msecs_to_jiffies(10)   /* [ms] */
+#define TIPC_UC_RETR_TIME (jiffies + msecs_to_jiffies(1))
 
 /*
  * Interval between NACKs when packets arrive out of order
@@ -1305,6 +1306,10 @@ static void tipc_link_advance_transmq(struct tipc_link *l, u16 acked, u16 gap,
 			kfree_skb(skb);
 		} else if (less_eq(seqno, acked + gap)) {
 			/* retransmit skb */
+			if (time_before(jiffies, TIPC_SKB_CB(skb)->nxt_retr))
+				continue;
+			TIPC_SKB_CB(skb)->nxt_retr = TIPC_UC_RETR_TIME;
+
 			_skb = __pskb_copy(skb, MIN_H_SIZE, GFP_ATOMIC);
 			if (!_skb)
 				continue;
@@ -1380,6 +1385,7 @@ static int tipc_link_build_nack_msg(struct tipc_link *l,
 				    struct sk_buff_head *xmitq)
 {
 	u32 def_cnt = ++l->stats.deferred_recv;
+	u32 defq_len = skb_queue_len(&l->deferdq);
 	int match1, match2;
 
 	if (link_is_bc_rcvlink(l)) {
@@ -1390,7 +1396,7 @@ static int tipc_link_build_nack_msg(struct tipc_link *l,
 		return 0;
 	}
 
-	if ((skb_queue_len(&l->deferdq) == 1) || !(def_cnt % TIPC_NACK_INTV))
+	if (defq_len >= 3 && !((defq_len - 3) % 16))
 		tipc_link_build_proto_msg(l, STATE_MSG, 0, 0, 0, 0, 0, xmitq);
 	return 0;
 }
@@ -1404,29 +1410,29 @@ int tipc_link_rcv(struct tipc_link *l, struct sk_buff *skb,
 		  struct sk_buff_head *xmitq)
 {
 	struct sk_buff_head *defq = &l->deferdq;
-	struct tipc_msg *hdr;
+	struct tipc_msg *hdr = buf_msg(skb);
 	u16 seqno, rcv_nxt, win_lim;
 	int rc = 0;
 
+	/* Verify and update link state */
+	if (unlikely(msg_user(hdr) == LINK_PROTOCOL))
+		return tipc_link_proto_rcv(l, skb, xmitq);
+
+	/* Don't send probe at next timeout expiration */
+	l->silent_intv_cnt = 0;
+
 	do {
 		hdr = buf_msg(skb);
 		seqno = msg_seqno(hdr);
 		rcv_nxt = l->rcv_nxt;
 		win_lim = rcv_nxt + TIPC_MAX_LINK_WIN;
 
-		/* Verify and update link state */
-		if (unlikely(msg_user(hdr) == LINK_PROTOCOL))
-			return tipc_link_proto_rcv(l, skb, xmitq);
-
 		if (unlikely(!link_is_up(l))) {
 			if (l->state == LINK_ESTABLISHING)
 				rc = TIPC_LINK_UP_EVT;
 			goto drop;
 		}
 
-		/* Don't send probe at next timeout expiration */
-		l->silent_intv_cnt = 0;
-
 		/* Drop if outside receive window */
 		if (unlikely(less(seqno, rcv_nxt) || more(seqno, win_lim))) {
 			l->stats.duplicates++;
@@ -1457,7 +1463,7 @@ int tipc_link_rcv(struct tipc_link *l, struct sk_buff *skb,
 			rc |= tipc_link_build_state_msg(l, xmitq);
 		if (unlikely(rc & ~TIPC_LINK_SND_STATE))
 			break;
-	} while ((skb = __skb_dequeue(defq)));
+	} while ((skb = __tipc_skb_dequeue(defq, l->rcv_nxt)));
 
 	return rc;
 drop:
diff --git a/net/tipc/msg.h b/net/tipc/msg.h
index ec5c511a9c9c..8de02ad6e352 100644
--- a/net/tipc/msg.h
+++ b/net/tipc/msg.h
@@ -1151,4 +1151,25 @@ static inline void tipc_skb_queue_splice_tail_init(struct sk_buff_head *list,
 	tipc_skb_queue_splice_tail(&tmp, head);
 }
 
+/* __tipc_skb_dequeue() - dequeue the head skb according to expected seqno
+ * @list: list to be dequeued from
+ * @seqno: seqno of the expected msg
+ *
+ * returns skb dequeued from the list if its seqno is less than or equal to
+ * the expected one, otherwise the skb is still hold
+ *
+ * Note: must be used with appropriate locks held only
+ */
+static inline struct sk_buff *__tipc_skb_dequeue(struct sk_buff_head *list,
+						 u16 seqno)
+{
+	struct sk_buff *skb = skb_peek(list);
+
+	if (skb && less_eq(buf_seqno(skb), seqno)) {
+		__skb_unlink(skb, list);
+		return skb;
+	}
+	return NULL;
+}
+
 #endif
-- 
2.13.7

[tipc-discussion] [net-next 1/3] tipc: improve TIPC throughput by Gap ACK blocks

[Tuong L. <tuo...@de...>]

During unicast link transmission, it's observed very often that because
of one or a few lost/dis-ordered packets, the sending side will fastly
reach the send window limit and must wait for the packets to be arrived
at the receiving side or in the worst case, a retransmission must be
done first. The sending side cannot release a lot of subsequent packets
in its transmq even though all of them might have already been received
by the receiving side.
That is, one or two packets dis-ordered/lost and dozens of packets have
to wait, this obviously reduces the overall throughput!

This commit introduces an algorithm to overcome this by using "Gap ACK
blocks". Basically, a Gap ACK block will consist of <ack, gap> numbers
that describes the link deferdq where packets have been got by the
receiving side but with gaps, for example:

      link deferdq: [1 2 3 4      10 11      13 14 15       20]
--> Gap ACK blocks:       <4, 5>,   <11, 1>,      <15, 4>, <20, 0>

The Gap ACK blocks will be sent to the sending side along with the
traditional ACK or NACK message. Immediately when receiving the message
the sending side will now not only release from its transmq the packets
ack-ed by the ACK but also by the Gap ACK blocks! So, more packets can
be enqueued and transmitted.
In addition, the sending side can now do "multi-retransmissions"
according to the Gaps reported in the Gap ACK blocks.

The new algorithm as verified helps greatly improve the TIPC throughput
especially under packet loss condition.

So far, a maximum of 32 blocks is quite enough without any "Too few Gap
ACK blocks" reports with a 5.0% packet loss rate, however this number
can be increased in the furture if needed.

Also, the patch is backward compatible.

Acked-by: Ying Xue <yin...@wi...>
Acked-by: Jon Maloy <jon...@er...>
Signed-off-by: Tuong Lien <tuo...@de...>
---
 net/tipc/link.c | 134 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 net/tipc/msg.h  |  31 +++++++++++++
 net/tipc/node.h |   6 ++-
 3 files changed, 159 insertions(+), 12 deletions(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index 52d23b3ffaf5..5aee1ed23ba9 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -246,6 +246,10 @@ static int tipc_link_build_nack_msg(struct tipc_link *l,
 static void tipc_link_build_bc_init_msg(struct tipc_link *l,
 					struct sk_buff_head *xmitq);
 static bool tipc_link_release_pkts(struct tipc_link *l, u16 to);
+static u16 tipc_build_gap_ack_blks(struct tipc_link *l, void *data);
+static void tipc_link_advance_transmq(struct tipc_link *l, u16 acked, u16 gap,
+				      struct tipc_gap_ack_blks *ga,
+				      struct sk_buff_head *xmitq);
 
 /*
  *  Simple non-static link routines (i.e. referenced outside this file)
@@ -1226,6 +1230,102 @@ static bool tipc_link_release_pkts(struct tipc_link *l, u16 acked)
 	return released;
 }
 
+/* tipc_build_gap_ack_blks - build Gap ACK blocks
+ * @l: tipc link that data have come with gaps in sequence if any
+ * @data: data buffer to store the Gap ACK blocks after built
+ *
+ * returns the actual allocated memory size
+ */
+static u16 tipc_build_gap_ack_blks(struct tipc_link *l, void *data)
+{
+	struct sk_buff *skb = skb_peek(&l->deferdq);
+	struct tipc_gap_ack_blks *ga = data;
+	u16 len, expect, seqno = 0;
+	u8 n = 0;
+
+	if (!skb)
+		goto exit;
+
+	expect = buf_seqno(skb);
+	skb_queue_walk(&l->deferdq, skb) {
+		seqno = buf_seqno(skb);
+		if (unlikely(more(seqno, expect))) {
+			ga->gacks[n].ack = htons(expect - 1);
+			ga->gacks[n].gap = htons(seqno - expect);
+			if (++n >= MAX_GAP_ACK_BLKS) {
+				pr_info_ratelimited("Too few Gap ACK blocks!\n");
+				goto exit;
+			}
+		} else if (unlikely(less(seqno, expect))) {
+			pr_warn("Unexpected skb in deferdq!\n");
+			continue;
+		}
+		expect = seqno + 1;
+	}
+
+	/* last block */
+	ga->gacks[n].ack = htons(seqno);
+	ga->gacks[n].gap = 0;
+	n++;
+
+exit:
+	len = tipc_gap_ack_blks_sz(n);
+	ga->len = htons(len);
+	ga->gack_cnt = n;
+	return len;
+}
+
+/* tipc_link_advance_transmq - advance TIPC link transmq queue by releasing
+ *			       acked packets, also doing retransmissions if
+ *			       gaps found
+ * @l: tipc link with transmq queue to be advanced
+ * @acked: seqno of last packet acked by peer without any gaps before
+ * @gap: # of gap packets
+ * @ga: buffer pointer to Gap ACK blocks from peer
+ * @xmitq: queue for accumulating the retransmitted packets if any
+ */
+static void tipc_link_advance_transmq(struct tipc_link *l, u16 acked, u16 gap,
+				      struct tipc_gap_ack_blks *ga,
+				      struct sk_buff_head *xmitq)
+{
+	struct sk_buff *skb, *_skb, *tmp;
+	struct tipc_msg *hdr;
+	u16 bc_ack = l->bc_rcvlink->rcv_nxt - 1;
+	u16 ack = l->rcv_nxt - 1;
+	u16 seqno;
+	u16 n = 0;
+
+	skb_queue_walk_safe(&l->transmq, skb, tmp) {
+		seqno = buf_seqno(skb);
+
+next_gap_ack:
+		if (less_eq(seqno, acked)) {
+			/* release skb */
+			__skb_unlink(skb, &l->transmq);
+			kfree_skb(skb);
+		} else if (less_eq(seqno, acked + gap)) {
+			/* retransmit skb */
+			_skb = __pskb_copy(skb, MIN_H_SIZE, GFP_ATOMIC);
+			if (!_skb)
+				continue;
+			hdr = buf_msg(_skb);
+			msg_set_ack(hdr, ack);
+			msg_set_bcast_ack(hdr, bc_ack);
+			_skb->priority = TC_PRIO_CONTROL;
+			__skb_queue_tail(xmitq, _skb);
+			l->stats.retransmitted++;
+		} else {
+			/* retry with Gap ACK blocks if any */
+			if (!ga || n >= ga->gack_cnt)
+				break;
+			acked = ntohs(ga->gacks[n].ack);
+			gap = ntohs(ga->gacks[n].gap);
+			n++;
+			goto next_gap_ack;
+		}
+	}
+}
+
 /* tipc_link_build_state_msg: prepare link state message for transmission
  *
  * Note that sending of broadcast ack is coordinated among nodes, to reduce
@@ -1378,6 +1478,7 @@ static void tipc_link_build_proto_msg(struct tipc_link *l, int mtyp, bool probe,
 	struct tipc_mon_state *mstate = &l->mon_state;
 	int dlen = 0;
 	void *data;
+	u16 glen = 0;
 
 	/* Don't send protocol message during reset or link failover */
 	if (tipc_link_is_blocked(l))
@@ -1390,8 +1491,8 @@ static void tipc_link_build_proto_msg(struct tipc_link *l, int mtyp, bool probe,
 		rcvgap = buf_seqno(skb_peek(dfq)) - l->rcv_nxt;
 
 	skb = tipc_msg_create(LINK_PROTOCOL, mtyp, INT_H_SIZE,
-			      tipc_max_domain_size, l->addr,
-			      tipc_own_addr(l->net), 0, 0, 0);
+			      tipc_max_domain_size + MAX_GAP_ACK_BLKS_SZ,
+			      l->addr, tipc_own_addr(l->net), 0, 0, 0);
 	if (!skb)
 		return;
 
@@ -1418,9 +1519,11 @@ static void tipc_link_build_proto_msg(struct tipc_link *l, int mtyp, bool probe,
 		msg_set_bc_gap(hdr, link_bc_rcv_gap(bcl));
 		msg_set_probe(hdr, probe);
 		msg_set_is_keepalive(hdr, probe || probe_reply);
-		tipc_mon_prep(l->net, data, &dlen, mstate, l->bearer_id);
-		msg_set_size(hdr, INT_H_SIZE + dlen);
-		skb_trim(skb, INT_H_SIZE + dlen);
+		if (l->peer_caps & TIPC_GAP_ACK_BLOCK)
+			glen = tipc_build_gap_ack_blks(l, data);
+		tipc_mon_prep(l->net, data + glen, &dlen, mstate, l->bearer_id);
+		msg_set_size(hdr, INT_H_SIZE + glen + dlen);
+		skb_trim(skb, INT_H_SIZE + glen + dlen);
 		l->stats.sent_states++;
 		l->rcv_unacked = 0;
 	} else {
@@ -1590,6 +1693,7 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
 			       struct sk_buff_head *xmitq)
 {
 	struct tipc_msg *hdr = buf_msg(skb);
+	struct tipc_gap_ack_blks *ga = NULL;
 	u16 rcvgap = 0;
 	u16 ack = msg_ack(hdr);
 	u16 gap = msg_seq_gap(hdr);
@@ -1600,6 +1704,7 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
 	u16 dlen = msg_data_sz(hdr);
 	int mtyp = msg_type(hdr);
 	bool reply = msg_probe(hdr);
+	u16 glen = 0;
 	void *data;
 	char *if_name;
 	int rc = 0;
@@ -1697,7 +1802,17 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
 				rc = TIPC_LINK_UP_EVT;
 			break;
 		}
-		tipc_mon_rcv(l->net, data, dlen, l->addr,
+
+		/* Receive Gap ACK blocks from peer if any */
+		if (l->peer_caps & TIPC_GAP_ACK_BLOCK) {
+			ga = (struct tipc_gap_ack_blks *)data;
+			glen = ntohs(ga->len);
+			/* sanity check: if failed, ignore Gap ACK blocks */
+			if (glen != tipc_gap_ack_blks_sz(ga->gack_cnt))
+				ga = NULL;
+		}
+
+		tipc_mon_rcv(l->net, data + glen, dlen - glen, l->addr,
 			     &l->mon_state, l->bearer_id);
 
 		/* Send NACK if peer has sent pkts we haven't received yet */
@@ -1706,13 +1821,12 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
 		if (rcvgap || reply)
 			tipc_link_build_proto_msg(l, STATE_MSG, 0, reply,
 						  rcvgap, 0, 0, xmitq);
-		tipc_link_release_pkts(l, ack);
+
+		tipc_link_advance_transmq(l, ack, gap, ga, xmitq);
 
 		/* If NACK, retransmit will now start at right position */
-		if (gap) {
-			rc = tipc_link_retrans(l, l, ack + 1, ack + gap, xmitq);
+		if (gap)
 			l->stats.recv_nacks++;
-		}
 
 		tipc_link_advance_backlog(l, xmitq);
 		if (unlikely(!skb_queue_empty(&l->wakeupq)))
diff --git a/net/tipc/msg.h b/net/tipc/msg.h
index 528ba9241acc..ec5c511a9c9c 100644
--- a/net/tipc/msg.h
+++ b/net/tipc/msg.h
@@ -117,6 +117,37 @@ struct tipc_msg {
 	__be32 hdr[15];
 };
 
+/* struct tipc_gap_ack - TIPC Gap ACK block
+ * @ack: seqno of the last consecutive packet in link deferdq
+ * @gap: number of gap packets since the last ack
+ *
+ * E.g:
+ *       link deferdq: 1 2 3 4      10 11      13 14 15       20
+ * --> Gap ACK blocks:      <4, 5>,   <11, 1>,      <15, 4>, <20, 0>
+ */
+struct tipc_gap_ack {
+	__be16 ack;
+	__be16 gap;
+};
+
+/* struct tipc_gap_ack_blks
+ * @len: actual length of the record
+ * @gack_cnt: number of Gap ACK blocks in the record
+ * @gacks: array of Gap ACK blocks
+ */
+struct tipc_gap_ack_blks {
+	__be16 len;
+	u8 gack_cnt;
+	u8 reserved;
+	struct tipc_gap_ack gacks[];
+};
+
+#define tipc_gap_ack_blks_sz(n) (sizeof(struct tipc_gap_ack_blks) + \
+				 sizeof(struct tipc_gap_ack) * (n))
+
+#define MAX_GAP_ACK_BLKS	32
+#define MAX_GAP_ACK_BLKS_SZ	tipc_gap_ack_blks_sz(MAX_GAP_ACK_BLKS)
+
 static inline struct tipc_msg *buf_msg(struct sk_buff *skb)
 {
 	return (struct tipc_msg *)skb->data;
diff --git a/net/tipc/node.h b/net/tipc/node.h
index 2404225c5d58..c0bf49ea3de4 100644
--- a/net/tipc/node.h
+++ b/net/tipc/node.h
@@ -52,7 +52,8 @@ enum {
 	TIPC_BCAST_RCAST      = (1 << 4),
 	TIPC_NODE_ID128       = (1 << 5),
 	TIPC_LINK_PROTO_SEQNO = (1 << 6),
-	TIPC_MCAST_RBCTL      = (1 << 7)
+	TIPC_MCAST_RBCTL      = (1 << 7),
+	TIPC_GAP_ACK_BLOCK    = (1 << 8)
 };
 
 #define TIPC_NODE_CAPABILITIES (TIPC_SYN_BIT           |  \
@@ -62,7 +63,8 @@ enum {
 				TIPC_BLOCK_FLOWCTL     |   \
 				TIPC_NODE_ID128        |   \
 				TIPC_LINK_PROTO_SEQNO  |   \
-				TIPC_MCAST_RBCTL)
+				TIPC_MCAST_RBCTL       |   \
+				TIPC_GAP_ACK_BLOCK)
 #define INVALID_BEARER_ID -1
 
 void tipc_node_stop(struct net *net);
-- 
2.13.7

Re: [tipc-discussion] [net-next] tipc: add NULL pointer check

[Jon M. <jon...@er...>]

> -----Original Message-----
> From: Bo YU <tsu...@gm...>
> Sent: 3-Apr-19 09:10
> To: Jon Maloy <jon...@er...>
> Cc: Hoang Huu Le <hoa...@de...>; ma...@do...;
> yin...@wi...; tip...@li...;
> ne...@vg...
> Subject: Re: [net-next] tipc: add NULL pointer check
> 
> On Wed, Apr 3, 2019 at 8:55 PM Jon Maloy <jon...@er...>
> wrote:
> >
> > Acked-by: Jon Maloy <jon...@er...>
> >
> > Although "somehow" is not the good term here,- the reason is obvious
> when looking into tipc_sk_proto_rcv().
> If so,skb_peek in tipc_mcast_xmit() maybe alert?

Don't understand your question. tipc_mcast_xmit() is on the send path,  we are on the receive path here.
Besides, this is a completely normal occurrence, so no need for any warning.

///jon
 
> >
> >
> > > -----Original Message-----
> > > From: net...@vg... <netdev-
> ow...@vg...> On
> > > Behalf Of Hoang Le
> > > Sent: 3-Apr-19 02:05
> > > To: Jon Maloy <jon...@er...>; ma...@do...;
> > > yin...@wi...; tip...@li...;
> > > ne...@vg...
> > > Subject: [net-next] tipc: add NULL pointer check
> > >
> > > skb somehow dequeued out of inputq before processing, it causes to
> > > NULL pointer and kernel crashed.
> > >
> > > Add checking skb valid before using.
> > >
> > > Fixes: c55c8edafa9 ("tipc: smooth change between replicast and
> > > broadcast")
> > > Reported-by: Tuong Lien Tong <tuo...@de...>
> > > Acked-by: Ying Xue <yin...@wi...>
> > > Signed-off-by: Hoang Le <hoa...@de...>
> > > ---
> > >  net/tipc/bcast.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c index
> > > 76e14dc08bb9..6c997d4a6218 100644
> > > --- a/net/tipc/bcast.c
> > > +++ b/net/tipc/bcast.c
> > > @@ -769,6 +769,9 @@ void tipc_mcast_filter_msg(struct net *net,
> > > struct sk_buff_head *defq,
> > >       u32 node, port;
> > >
> > >       skb = skb_peek(inputq);
> > > +     if (!skb)
> > > +             return;
> > > +
> > >       hdr = buf_msg(skb);
> > >
> > >       if (likely(!msg_is_syn(hdr) && skb_queue_empty(defq)))
> > > --
> > > 2.17.1
> >

Re: [tipc-discussion] [net-next] tipc: add NULL pointer check

[Jon M. <jon...@er...>]

Acked-by: Jon Maloy <jon...@er...>

Although "somehow" is not the good term here,- the reason is obvious when looking into tipc_sk_proto_rcv().


> -----Original Message-----
> From: net...@vg... <net...@vg...>
> On Behalf Of Hoang Le
> Sent: 3-Apr-19 02:05
> To: Jon Maloy <jon...@er...>; ma...@do...;
> yin...@wi...; tip...@li...;
> ne...@vg...
> Subject: [net-next] tipc: add NULL pointer check
> 
> skb somehow dequeued out of inputq before processing, it causes to NULL
> pointer and kernel crashed.
> 
> Add checking skb valid before using.
> 
> Fixes: c55c8edafa9 ("tipc: smooth change between replicast and broadcast")
> Reported-by: Tuong Lien Tong <tuo...@de...>
> Acked-by: Ying Xue <yin...@wi...>
> Signed-off-by: Hoang Le <hoa...@de...>
> ---
>  net/tipc/bcast.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c index
> 76e14dc08bb9..6c997d4a6218 100644
> --- a/net/tipc/bcast.c
> +++ b/net/tipc/bcast.c
> @@ -769,6 +769,9 @@ void tipc_mcast_filter_msg(struct net *net, struct
> sk_buff_head *defq,
>  	u32 node, port;
> 
>  	skb = skb_peek(inputq);
> +	if (!skb)
> +		return;
> +
>  	hdr = buf_msg(skb);
> 
>  	if (likely(!msg_is_syn(hdr) && skb_queue_empty(defq)))
> --
> 2.17.1