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

[tipc-discussion] [net ] tipc: add stricter control of reserved service types

[<jm...@re...>]

From: Jon Maloy <jm...@re...>

TIPC reserves 64 service types for current and future internal use.
Therefore, the bind() function is meant to block regular user sockets
from being bound to these values, while it should let through such
bindings from internal users.

However, since we at the design moment saw no way to distinguish
between regular and internal users the filter function ended up
with allowing all bindings of the types which were really in use
([0,1]), and block all the rest ([2,63]).

This is dangerous, since a regular user may bind to the service type
representing the topology server (TIPC_TOP_SRV == 1) or the one used
for indicating neigboring node status (TIPC_CFG_SRV == 0), and wreak
havoc for users of those services. I.e., practically all users.

The reality is however that TIPC_CFG_SRV never is bound through the
bind() function, since it doesn't represent a regular socket, and
TIPC_TOP_SRV can easily be filtered out, since it is the very first
binding performed when the system is starting.

We can hence block TIPC_CFG_SRV completely, and only allow TIPC_TOP_SRV
to be bound once, and the correct behavior is achieved. This is what we
do in this commit.

It should be noted that, although this is a change of the API semantics,
there is no risk we will break any currently working applications by
doing this. Any application trying to bind to the values in question
would be badly broken from the outset, so there is no chance we would
find any such applications in real-world production systems.

Signed-off-by: Jon Maloy <jm...@re...>
---
 net/tipc/socket.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index e795a8a2955b..67875a5761d0 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -665,6 +665,7 @@ static int tipc_bind(struct socket *sock, struct sockaddr *uaddr,
 	struct sockaddr_tipc *addr = (struct sockaddr_tipc *)uaddr;
 	struct tipc_sock *tsk = tipc_sk(sk);
 	int res = -EINVAL;
+	u32 stype, dnode;
 
 	lock_sock(sk);
 	if (unlikely(!uaddr_len)) {
@@ -691,9 +692,10 @@ static int tipc_bind(struct socket *sock, struct sockaddr *uaddr,
 		goto exit;
 	}
 
-	if ((addr->addr.nameseq.type < TIPC_RESERVED_TYPES) &&
-	    (addr->addr.nameseq.type != TIPC_TOP_SRV) &&
-	    (addr->addr.nameseq.type != TIPC_CFG_SRV)) {
+	stype = addr->addr.nameseq.type;
+	if (stype < TIPC_RESERVED_TYPES &&
+	    (stype != TIPC_TOP_SRV ||
+	     tipc_nametbl_translate(sock_net(sk), stype, stype, &dnode))) {
 		res = -EACCES;
 		goto exit;
 	}
-- 
2.25.4

Re: [tipc-discussion] [net] tipc: fix NULL pointer dereference in tipc_named_rcv

[Jon M. <jm...@re...>]

On 10/9/20 12:12 AM, Hoang Huu Le wrote:
> Hi Jon,  Jakub,
>
> I tried with your comment. But looks like we got into circular locking and deadlock could happen like this:
>          CPU0                    CPU1
>          ----                    ----
>     lock(&n->lock#2);
>                                  lock(&tn->nametbl_lock);
>                                  lock(&n->lock#2);
>     lock(&tn->nametbl_lock);
>
>    *** DEADLOCK ***
>
> Regards,
> Hoang
Ok. So although your solution is not optimal, we know it is safe.
Again:
Acked-by: Jon Maloy <jm...@re...>
>> -----Original Message-----
>> From: Jon Maloy <jm...@re...>
>> Sent: Friday, October 9, 2020 1:01 AM
>> To: Jakub Kicinski <ku...@ke...>; Hoang Huu Le <hoa...@de...>
>> Cc: ma...@do...; yin...@wi...; tip...@li...; ne...@vg...
>> Subject: Re: [net] tipc: fix NULL pointer dereference in tipc_named_rcv
>>
>>
>>
>> On 10/8/20 1:25 PM, Jakub Kicinski wrote:
>>> On Thu,  8 Oct 2020 14:31:56 +0700 Hoang Huu Le wrote:
>>>> diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
>>>> index 2f9c148f17e2..fe4edce459ad 100644
>>>> --- a/net/tipc/name_distr.c
>>>> +++ b/net/tipc/name_distr.c
>>>> @@ -327,8 +327,13 @@ static struct sk_buff *tipc_named_dequeue(struct sk_buff_head *namedq,
>>>>    	struct tipc_msg *hdr;
>>>>    	u16 seqno;
>>>>
>>>> +	spin_lock_bh(&namedq->lock);
>>>>    	skb_queue_walk_safe(namedq, skb, tmp) {
>>>> -		skb_linearize(skb);
>>>> +		if (unlikely(skb_linearize(skb))) {
>>>> +			__skb_unlink(skb, namedq);
>>>> +			kfree_skb(skb);
>>>> +			continue;
>>>> +		}
>>>>    		hdr = buf_msg(skb);
>>>>    		seqno = msg_named_seqno(hdr);
>>>>    		if (msg_is_last_bulk(hdr)) {
>>>> @@ -338,12 +343,14 @@ static struct sk_buff *tipc_named_dequeue(struct sk_buff_head *namedq,
>>>>
>>>>    		if (msg_is_bulk(hdr) || msg_is_legacy(hdr)) {
>>>>    			__skb_unlink(skb, namedq);
>>>> +			spin_unlock_bh(&namedq->lock);
>>>>    			return skb;
>>>>    		}
>>>>
>>>>    		if (*open && (*rcv_nxt == seqno)) {
>>>>    			(*rcv_nxt)++;
>>>>    			__skb_unlink(skb, namedq);
>>>> +			spin_unlock_bh(&namedq->lock);
>>>>    			return skb;
>>>>    		}
>>>>
>>>> @@ -353,6 +360,7 @@ static struct sk_buff *tipc_named_dequeue(struct sk_buff_head *namedq,
>>>>    			continue;
>>>>    		}
>>>>    	}
>>>> +	spin_unlock_bh(&namedq->lock);
>>>>    	return NULL;
>>>>    }
>>>>
>>>> diff --git a/net/tipc/node.c b/net/tipc/node.c
>>>> index cf4b239fc569..d269ebe382e1 100644
>>>> --- a/net/tipc/node.c
>>>> +++ b/net/tipc/node.c
>>>> @@ -1496,7 +1496,7 @@ static void node_lost_contact(struct tipc_node *n,
>>>>
>>>>    	/* Clean up broadcast state */
>>>>    	tipc_bcast_remove_peer(n->net, n->bc_entry.link);
>>>> -	__skb_queue_purge(&n->bc_entry.namedq);
>>>> +	skb_queue_purge(&n->bc_entry.namedq);
>>> Patch looks fine, but I'm not sure why not hold
>>> spin_unlock_bh(&tn->nametbl_lock) here instead?
>>>
>>> Seems like node_lost_contact() should be relatively rare,
>>> so adding another lock to tipc_named_dequeue() is not the
>>> right trade off.
>> Actually, I agree with previous speaker here. We already have the
>> nametbl_lock when tipc_named_dequeue() is called, and the same lock is
>> accessible from no.c where node_lost_contact() is executed. The patch
>> and the code becomes simpler.
>> I suggest you post a v2 of this one.
>>
>> ///jon
>>
>>>>    	/* Abort any ongoing link failover */
>>>>    	for (i = 0; i < MAX_BEARERS; i++) {

[tipc-discussion] [net-next] tipc: remove dead code in tipc_net and relatives

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

dist_queue is no longer used since commit 37922ea4a310
("tipc: permit overlapping service ranges in name table")

Signed-off-by: Hoang Huu Le <hoa...@de...>
---
 net/tipc/core.c       |  2 --
 net/tipc/core.h       |  3 ---
 net/tipc/name_distr.c | 19 -------------------
 3 files changed, 24 deletions(-)

diff --git a/net/tipc/core.c b/net/tipc/core.c
index c2ff42900b53..5cc1f0307215 100644
--- a/net/tipc/core.c
+++ b/net/tipc/core.c
@@ -81,8 +81,6 @@ static int __net_init tipc_init_net(struct net *net)
 	if (err)
 		goto out_nametbl;
 
-	INIT_LIST_HEAD(&tn->dist_queue);
-
 	err = tipc_bcast_init(net);
 	if (err)
 		goto out_bclink;
diff --git a/net/tipc/core.h b/net/tipc/core.h
index 1d57a4d3b05e..df34dcdd0607 100644
--- a/net/tipc/core.h
+++ b/net/tipc/core.h
@@ -132,9 +132,6 @@ struct tipc_net {
 	spinlock_t nametbl_lock;
 	struct name_table *nametbl;
 
-	/* Name dist queue */
-	struct list_head dist_queue;
-
 	/* Topology subscription server */
 	struct tipc_topsrv *topsrv;
 	atomic_t subscription_count;
diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
index 2f9c148f17e2..4d50798fe36c 100644
--- a/net/tipc/name_distr.c
+++ b/net/tipc/name_distr.c
@@ -244,24 +244,6 @@ static void tipc_publ_purge(struct net *net, struct publication *publ, u32 addr)
 		kfree_rcu(p, rcu);
 }
 
-/**
- * tipc_dist_queue_purge - remove deferred updates from a node that went down
- */
-static void tipc_dist_queue_purge(struct net *net, u32 addr)
-{
-	struct tipc_net *tn = net_generic(net, tipc_net_id);
-	struct distr_queue_item *e, *tmp;
-
-	spin_lock_bh(&tn->nametbl_lock);
-	list_for_each_entry_safe(e, tmp, &tn->dist_queue, next) {
-		if (e->node != addr)
-			continue;
-		list_del(&e->next);
-		kfree(e);
-	}
-	spin_unlock_bh(&tn->nametbl_lock);
-}
-
 void tipc_publ_notify(struct net *net, struct list_head *nsub_list,
 		      u32 addr, u16 capabilities)
 {
@@ -272,7 +254,6 @@ void tipc_publ_notify(struct net *net, struct list_head *nsub_list,
 
 	list_for_each_entry_safe(publ, tmp, nsub_list, binding_node)
 		tipc_publ_purge(net, publ, addr);
-	tipc_dist_queue_purge(net, addr);
 	spin_lock_bh(&tn->nametbl_lock);
 	if (!(capabilities & TIPC_NAMED_BCAST))
 		nt->rc_dests--;
-- 
2.25.1

Re: [tipc-discussion] [net] tipc: fix NULL pointer dereference in tipc_named_rcv

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

Hi Jon,  Jakub,

I tried with your comment. But looks like we got into circular locking and deadlock could happen like this:
        CPU0                    CPU1
        ----                    ----
   lock(&n->lock#2);
                                lock(&tn->nametbl_lock);
                                lock(&n->lock#2);
   lock(&tn->nametbl_lock);

  *** DEADLOCK ***

Regards,
Hoang
> -----Original Message-----
> From: Jon Maloy <jm...@re...>
> Sent: Friday, October 9, 2020 1:01 AM
> To: Jakub Kicinski <ku...@ke...>; Hoang Huu Le <hoa...@de...>
> Cc: ma...@do...; yin...@wi...; tip...@li...; ne...@vg...
> Subject: Re: [net] tipc: fix NULL pointer dereference in tipc_named_rcv
> 
> 
> 
> On 10/8/20 1:25 PM, Jakub Kicinski wrote:
> > On Thu,  8 Oct 2020 14:31:56 +0700 Hoang Huu Le wrote:
> >> diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
> >> index 2f9c148f17e2..fe4edce459ad 100644
> >> --- a/net/tipc/name_distr.c
> >> +++ b/net/tipc/name_distr.c
> >> @@ -327,8 +327,13 @@ static struct sk_buff *tipc_named_dequeue(struct sk_buff_head *namedq,
> >>   	struct tipc_msg *hdr;
> >>   	u16 seqno;
> >>
> >> +	spin_lock_bh(&namedq->lock);
> >>   	skb_queue_walk_safe(namedq, skb, tmp) {
> >> -		skb_linearize(skb);
> >> +		if (unlikely(skb_linearize(skb))) {
> >> +			__skb_unlink(skb, namedq);
> >> +			kfree_skb(skb);
> >> +			continue;
> >> +		}
> >>   		hdr = buf_msg(skb);
> >>   		seqno = msg_named_seqno(hdr);
> >>   		if (msg_is_last_bulk(hdr)) {
> >> @@ -338,12 +343,14 @@ static struct sk_buff *tipc_named_dequeue(struct sk_buff_head *namedq,
> >>
> >>   		if (msg_is_bulk(hdr) || msg_is_legacy(hdr)) {
> >>   			__skb_unlink(skb, namedq);
> >> +			spin_unlock_bh(&namedq->lock);
> >>   			return skb;
> >>   		}
> >>
> >>   		if (*open && (*rcv_nxt == seqno)) {
> >>   			(*rcv_nxt)++;
> >>   			__skb_unlink(skb, namedq);
> >> +			spin_unlock_bh(&namedq->lock);
> >>   			return skb;
> >>   		}
> >>
> >> @@ -353,6 +360,7 @@ static struct sk_buff *tipc_named_dequeue(struct sk_buff_head *namedq,
> >>   			continue;
> >>   		}
> >>   	}
> >> +	spin_unlock_bh(&namedq->lock);
> >>   	return NULL;
> >>   }
> >>
> >> diff --git a/net/tipc/node.c b/net/tipc/node.c
> >> index cf4b239fc569..d269ebe382e1 100644
> >> --- a/net/tipc/node.c
> >> +++ b/net/tipc/node.c
> >> @@ -1496,7 +1496,7 @@ static void node_lost_contact(struct tipc_node *n,
> >>
> >>   	/* Clean up broadcast state */
> >>   	tipc_bcast_remove_peer(n->net, n->bc_entry.link);
> >> -	__skb_queue_purge(&n->bc_entry.namedq);
> >> +	skb_queue_purge(&n->bc_entry.namedq);
> > Patch looks fine, but I'm not sure why not hold
> > spin_unlock_bh(&tn->nametbl_lock) here instead?
> >
> > Seems like node_lost_contact() should be relatively rare,
> > so adding another lock to tipc_named_dequeue() is not the
> > right trade off.
> Actually, I agree with previous speaker here. We already have the
> nametbl_lock when tipc_named_dequeue() is called, and the same lock is
> accessible from no.c where node_lost_contact() is executed. The patch
> and the code becomes simpler.
> I suggest you post a v2 of this one.
> 
> ///jon
> 
> >>   	/* Abort any ongoing link failover */
> >>   	for (i = 0; i < MAX_BEARERS; i++) {

Re: [tipc-discussion] [net-next] tipc: introduce smooth change to replicast

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

Hi Jon,

See my inline comment.

Thanks,
Hoang
> -----Original Message-----
> From: Jon Maloy <jm...@re...>
> Sent: Friday, October 9, 2020 1:27 AM
> To: Hoang Huu Le <hoa...@de...>; ma...@do...; yin...@wi...; tipc-
> dis...@li...
> Subject: Re: [net-next] tipc: introduce smooth change to replicast
> 
> 
> 
> On 10/7/20 10:22 PM, Hoang Huu Le wrote:
> > In commit cad2929dc432 ("tipc: update a binding service via broadcast"),
> > We use broadcast to update a binding service for a large cluster.
> > However, if we try to publish a thousands of services at the
> > same time, we may to get "link overflow" happen because of queue limit
> > had reached.
> >
> > We now introduce a smooth change to replicast if the broadcast link has
> > reach to the limit of queue.
> To me this beats the whole purpose of using broadcast distribution in
> the first place.
> We wanted to save CPU and network resources by using  broadcast, and
> then, when things get tough, we fall back to the supposedly less
> efficient replicast method. Not good.
> 
> I wonder what is really happening when this overflow situation occurs.
> First, the reset limit is dimensioned so that it should be possible to
> publish MAX_PUBLICATIONS (65k) publications in one shot.
> With full bundling, which is what I expect here, there are 1460/20 = 73
> publication items in each buffer, so the reset limit (==max_bulk) should
> be 65k/73 = 897 buffers.

[Hoang] No, it's not right!
As I staged in another commit that the reset limit is 350 buffers (65k/(3744/20)) => #1.
where:
FB_MTU=3744
and if a user set window size is 50, we are fallback to 32 window-size => #2. 
So, if the broadcast link is under high load traffic, we will reach to the limit easily (#1 + #2).

> My figures are just from the top of my head, so you should double check
> them, but I find it unlikely that we hit this limit unless there is a
> lot of other broadcast going on at the same time, and even then I find
> it unlikely.
[Hoang]
I just implement a simple application:
[...]
num_service = 10k
for (i=0;i<num_service; i++)
    bind(socket, service<i>);
[...]

Then, run this app; sleep 2; kill SIGINT app; Then, the problem is reproducible.
 
> I suggest you try to find out what is really going on when we reach this
> situation.
> -What exactly is in the backlog queue?
> -Only publications?
> -How many?
> -A mixture of publications and other traffic?
Only publication/withdrawn buffers, around more thousands.

> -Has bundling really worked as supposed?
> -Do we still have some issue with the broadcast link that stops buffers
> being acked and released in a timely manner?
> - Have you been able to dump out such info when this problem occurs?
> - Are you able to re-produce it in your own system?
These answers are YES

> In the end it might be as simple as increasing the reset limit, but we
> really should try to understand what is happening first.
Yes, I think so. As previous patch I made the code change to update queue backlog supporting to 873 buffers.
But if I increase number of services in my app up to 20k (not real??>) . The issue is able to reproduce as well.
That is the reason why I propose this new solution + combine with two previous patches to solve the problem completely.
> 
> Regards
> ///jon
> 
> 
> >
> > Signed-off-by: Hoang Huu Le <hoa...@de...>
> > ---
> >   net/tipc/link.c |  5 ++++-
> >   net/tipc/node.c | 12 ++++++++++--
> >   2 files changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/tipc/link.c b/net/tipc/link.c
> > index 06b880da2a8e..ca908ead753a 100644
> > --- a/net/tipc/link.c
> > +++ b/net/tipc/link.c
> > @@ -1022,7 +1022,10 @@ int tipc_link_xmit(struct tipc_link *l, struct sk_buff_head *list,
> >   	/* Allow oversubscription of one data msg per source at congestion */
> >   	if (unlikely(l->backlog[imp].len >= l->backlog[imp].limit)) {
> >   		if (imp == TIPC_SYSTEM_IMPORTANCE) {
> > -			pr_warn("%s<%s>, link overflow", link_rst_msg, l->name);
> > +			pr_warn_ratelimited("%s<%s>, link overflow",
> > +					    link_rst_msg, l->name);
> > +			if (link_is_bc_sndlink(l))
> > +				return -EOVERFLOW;
> >   			return -ENOBUFS;
> >   		}
> >   		rc = link_schedule_user(l, hdr);
> > diff --git a/net/tipc/node.c b/net/tipc/node.c
> > index d269ebe382e1..a37976610367 100644
> > --- a/net/tipc/node.c
> > +++ b/net/tipc/node.c
> > @@ -1750,15 +1750,23 @@ void tipc_node_broadcast(struct net *net, struct sk_buff *skb, int rc_dests)
> >   	struct tipc_node *n;
> >   	u16 dummy;
> >   	u32 dst;
> > +	int rc = 0;
> >
> >   	/* Use broadcast if all nodes support it */
> >   	if (!rc_dests && tipc_bcast_get_mode(net) != BCLINK_MODE_RCAST) {
> > +		txskb = pskb_copy(skb, GFP_ATOMIC);
> > +		if (!txskb)
> > +			goto rcast;
> >   		__skb_queue_head_init(&xmitq);
> > -		__skb_queue_tail(&xmitq, skb);
> > -		tipc_bcast_xmit(net, &xmitq, &dummy);
> > +		__skb_queue_tail(&xmitq, txskb);
> > +		rc = tipc_bcast_xmit(net, &xmitq, &dummy);
> > +		if (rc == -EOVERFLOW)
> > +			goto rcast;
> > +		kfree_skb(skb);
> >   		return;
> >   	}
> >
> > +rcast:
> >   	/* Otherwise use legacy replicast method */
> >   	rcu_read_lock();
> >   	list_for_each_entry_rcu(n, tipc_nodes(net), list) {

Re: [tipc-discussion] [net-next] tipc: introduce smooth change to replicast

[Jon M. <jm...@re...>]

On 10/7/20 10:22 PM, Hoang Huu Le wrote:
> In commit cad2929dc432 ("tipc: update a binding service via broadcast"),
> We use broadcast to update a binding service for a large cluster.
> However, if we try to publish a thousands of services at the
> same time, we may to get "link overflow" happen because of queue limit
> had reached.
>
> We now introduce a smooth change to replicast if the broadcast link has
> reach to the limit of queue.
To me this beats the whole purpose of using broadcast distribution in 
the first place.
We wanted to save CPU and network resources by using  broadcast, and 
then, when things get tough, we fall back to the supposedly less 
efficient replicast method. Not good.

I wonder what is really happening when this overflow situation occurs.
First, the reset limit is dimensioned so that it should be possible to 
publish MAX_PUBLICATIONS (65k) publications in one shot.
With full bundling, which is what I expect here, there are 1460/20 = 73 
publication items in each buffer, so the reset limit (==max_bulk) should 
be 65k/73 = 897 buffers.
My figures are just from the top of my head, so you should double check 
them, but I find it unlikely that we hit this limit unless there is a 
lot of other broadcast going on at the same time, and even then I find 
it unlikely.

I suggest you try to find out what is really going on when we reach this 
situation.
-What exactly is in the backlog queue?
-Only publications?
-How many?
-A mixture of publications and other traffic?
-Has bundling really worked as supposed?
-Do we still have some issue with the broadcast link that stops buffers 
being acked and released in a timely manner?
- Have you been able to dump out such info when this problem occurs?
- Are you able to re-produce it in your own system?

In the end it might be as simple as increasing the reset limit, but we 
really should try to understand what is happening first.

Regards
///jon


>
> Signed-off-by: Hoang Huu Le <hoa...@de...>
> ---
>   net/tipc/link.c |  5 ++++-
>   net/tipc/node.c | 12 ++++++++++--
>   2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/net/tipc/link.c b/net/tipc/link.c
> index 06b880da2a8e..ca908ead753a 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -1022,7 +1022,10 @@ int tipc_link_xmit(struct tipc_link *l, struct sk_buff_head *list,
>   	/* Allow oversubscription of one data msg per source at congestion */
>   	if (unlikely(l->backlog[imp].len >= l->backlog[imp].limit)) {
>   		if (imp == TIPC_SYSTEM_IMPORTANCE) {
> -			pr_warn("%s<%s>, link overflow", link_rst_msg, l->name);
> +			pr_warn_ratelimited("%s<%s>, link overflow",
> +					    link_rst_msg, l->name);
> +			if (link_is_bc_sndlink(l))
> +				return -EOVERFLOW;
>   			return -ENOBUFS;
>   		}
>   		rc = link_schedule_user(l, hdr);
> diff --git a/net/tipc/node.c b/net/tipc/node.c
> index d269ebe382e1..a37976610367 100644
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -1750,15 +1750,23 @@ void tipc_node_broadcast(struct net *net, struct sk_buff *skb, int rc_dests)
>   	struct tipc_node *n;
>   	u16 dummy;
>   	u32 dst;
> +	int rc = 0;
>   
>   	/* Use broadcast if all nodes support it */
>   	if (!rc_dests && tipc_bcast_get_mode(net) != BCLINK_MODE_RCAST) {
> +		txskb = pskb_copy(skb, GFP_ATOMIC);
> +		if (!txskb)
> +			goto rcast;
>   		__skb_queue_head_init(&xmitq);
> -		__skb_queue_tail(&xmitq, skb);
> -		tipc_bcast_xmit(net, &xmitq, &dummy);
> +		__skb_queue_tail(&xmitq, txskb);
> +		rc = tipc_bcast_xmit(net, &xmitq, &dummy);
> +		if (rc == -EOVERFLOW)
> +			goto rcast;
> +		kfree_skb(skb);
>   		return;
>   	}
>   
> +rcast:
>   	/* Otherwise use legacy replicast method */
>   	rcu_read_lock();
>   	list_for_each_entry_rcu(n, tipc_nodes(net), list) {

Re: [tipc-discussion] [net] tipc: fix NULL pointer dereference in tipc_named_rcv

[Jon M. <jm...@re...>]

On 10/8/20 1:25 PM, Jakub Kicinski wrote:
> On Thu,  8 Oct 2020 14:31:56 +0700 Hoang Huu Le wrote:
>> diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
>> index 2f9c148f17e2..fe4edce459ad 100644
>> --- a/net/tipc/name_distr.c
>> +++ b/net/tipc/name_distr.c
>> @@ -327,8 +327,13 @@ static struct sk_buff *tipc_named_dequeue(struct sk_buff_head *namedq,
>>   	struct tipc_msg *hdr;
>>   	u16 seqno;
>>   
>> +	spin_lock_bh(&namedq->lock);
>>   	skb_queue_walk_safe(namedq, skb, tmp) {
>> -		skb_linearize(skb);
>> +		if (unlikely(skb_linearize(skb))) {
>> +			__skb_unlink(skb, namedq);
>> +			kfree_skb(skb);
>> +			continue;
>> +		}
>>   		hdr = buf_msg(skb);
>>   		seqno = msg_named_seqno(hdr);
>>   		if (msg_is_last_bulk(hdr)) {
>> @@ -338,12 +343,14 @@ static struct sk_buff *tipc_named_dequeue(struct sk_buff_head *namedq,
>>   
>>   		if (msg_is_bulk(hdr) || msg_is_legacy(hdr)) {
>>   			__skb_unlink(skb, namedq);
>> +			spin_unlock_bh(&namedq->lock);
>>   			return skb;
>>   		}
>>   
>>   		if (*open && (*rcv_nxt == seqno)) {
>>   			(*rcv_nxt)++;
>>   			__skb_unlink(skb, namedq);
>> +			spin_unlock_bh(&namedq->lock);
>>   			return skb;
>>   		}
>>   
>> @@ -353,6 +360,7 @@ static struct sk_buff *tipc_named_dequeue(struct sk_buff_head *namedq,
>>   			continue;
>>   		}
>>   	}
>> +	spin_unlock_bh(&namedq->lock);
>>   	return NULL;
>>   }
>>   
>> diff --git a/net/tipc/node.c b/net/tipc/node.c
>> index cf4b239fc569..d269ebe382e1 100644
>> --- a/net/tipc/node.c
>> +++ b/net/tipc/node.c
>> @@ -1496,7 +1496,7 @@ static void node_lost_contact(struct tipc_node *n,
>>   
>>   	/* Clean up broadcast state */
>>   	tipc_bcast_remove_peer(n->net, n->bc_entry.link);
>> -	__skb_queue_purge(&n->bc_entry.namedq);
>> +	skb_queue_purge(&n->bc_entry.namedq);
> Patch looks fine, but I'm not sure why not hold
> spin_unlock_bh(&tn->nametbl_lock) here instead?
>
> Seems like node_lost_contact() should be relatively rare,
> so adding another lock to tipc_named_dequeue() is not the
> right trade off.
Actually, I agree with previous speaker here. We already have the 
nametbl_lock when tipc_named_dequeue() is called, and the same lock is 
accessible from no.c where node_lost_contact() is executed. The patch 
and the code becomes simpler.
I suggest you post a v2 of this one.

///jon

>>   	/* Abort any ongoing link failover */
>>   	for (i = 0; i < MAX_BEARERS; i++) {

Re: [tipc-discussion] tipc.py

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

Hi Jon,

I've done at:
https://sourceforge.net/p/tipc/tipcutils/merge-requests/8/

Please take time to review it.

Regards,
Hoang
> -----Original Message-----
> From: Jon Maloy <jm...@re...>
> Sent: Monday, October 5, 2020 10:44 PM
> To: Hoang Huu Le <hoa...@de...>; tip...@li...; tipc-dek <tip...@de...>; Xin
> Long <luc...@gm...>; Ying Xue <yin...@wi...>
> Subject: Re: tipc.py
> 
> 
> 
> On 10/5/20 12:35 AM, Hoang Huu Le wrote:
> > Hi Jon,
> >
> > I will make an effort on this. Do you think we need to keep these APIs compatibility work with Python2 or completely remove out?
> Great! To me you can just update the current code and don't worry about
> compatibility.
> If anybody has used the current module they will have their own copy,
> and I cannot imagine that anybody will write new programs for Puthon 2
> by now.
> 
> Regards
> ///jon
> 
> >
> > Regards,
> > Hoang
> >> -----Original Message-----
> >> From: Jon Maloy <jm...@re...>
> >> Sent: Saturday, October 3, 2020 5:59 AM
> >> To: Hoang Huu Le <hoa...@de...>; tip...@li...; tipc-dek <tip...@de...>;
> Xin
> >> Long <luc...@gm...>; Ying Xue <yin...@wi...>
> >> Subject: Re: tipc.py
> >>
> >>
> >>
> >> On 10/1/20 11:04 PM, Hoang Huu Le wrote:
> >>> Hi Jon,
> >>>
> >>> I've done this a long time ago:
> >>> 5057f8bb4de0 tipcutils: introduce python api
> >>>
> >>> Basically, it works with Python 2.
> >> Do you think it you would have time to do this for Python 3?
> >> Python 2 is practically dead now, as we all know.
> >>
> >> Regards
> >> ///jon
> >>> Regards,
> >>> Hoang
> >>>> -----Original Message-----
> >>>> From: Jon Maloy <jm...@re...>
> >>>> Sent: Friday, October 2, 2020 4:56 AM
> >>>> To: tip...@li...; tipc-dek <tip...@de...>; Xin Long <luc...@gm...>; Ying Xue
> >>>> <yin...@wi...>
> >>>> Subject: tipc.py
> >>>>
> >>>> I am updating the programmer's manual, and realized that the Python API
> >>>> is missing.
> >>>> Since there are so many programmers knowing Python nowadays, but not C,
> >>>> I think it would
> >>>> be very useful to have this API in the manual, so those programmers can
> >>>> get a feeling
> >>>> for how simple it to use TIPC.
> >>>>
> >>>> Tuong started development of an API based on the libtipc C-API, but it
> >>>> seems to me it was never finished.
> >>>> However, Python does since a long time have native TIPC support,
> >>>> allegedly both in Python 2 and Pyton 3.
> >>>> I had never seen that API until now, but after some googling I came over
> >>>> the following link, that seems to contain
> >>>> that native implemenation:
> >>>>
> >>>> https://blitiri.com.ar/p/pytipc/
> >>>>
> >>>> I wonder if anybody has the time to try this one, and verify, using the
> >>>> examples, that it really works.
> >>>> It would be embarrassing to add this to the manual if it turns out it
> >>>> doesn't work.
> >>>>
> >>>> Regards
> >>>> ///jon
> >>>>
> >>>> PS. Does anybody volunteer to be become co-maintainer of the home page
> >>>> and project page
> >>>>          at SourceForge? I think we should even consider moving it to
> >>>> GitLab or GitHub.
> >>>>          Since we have our own domain (tipc.io) that could easily be
> >>>> re-steered to a different
> >>>>          host system.

[tipc-discussion] [net] tipc: fix NULL pointer dereference in tipc_named_rcv

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

In the function node_lost_contact(), we call __skb_queue_purge() without
grabbing the list->lock. This can cause to a race-condition why processing
the list 'namedq' in calling path tipc_named_rcv()->tipc_named_dequeue().

    [] BUG: kernel NULL pointer dereference, address: 0000000000000000
    [] #PF: supervisor read access in kernel mode
    [] #PF: error_code(0x0000) - not-present page
    [] PGD 7ca63067 P4D 7ca63067 PUD 6c553067 PMD 0
    [] Oops: 0000 [#1] SMP NOPTI
    [] CPU: 1 PID: 15 Comm: ksoftirqd/1 Tainted: G  O  5.9.0-rc6+ #2
    [] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS [...]
    [] RIP: 0010:tipc_named_rcv+0x103/0x320 [tipc]
    [] Code: 41 89 44 24 10 49 8b 16 49 8b 46 08 49 c7 06 00 00 00 [...]
    [] RSP: 0018:ffffc900000a7c58 EFLAGS: 00000282
    [] RAX: 00000000000012ec RBX: 0000000000000000 RCX: ffff88807bde1270
    [] RDX: 0000000000002c7c RSI: 0000000000002c7c RDI: ffff88807b38f1a8
    [] RBP: ffff88807b006288 R08: ffff88806a367800 R09: ffff88806a367900
    [] R10: ffff88806a367a00 R11: ffff88806a367b00 R12: ffff88807b006258
    [] R13: ffff88807b00628a R14: ffff888069334d00 R15: ffff88806a434600
    [] FS:  0000000000000000(0000) GS:ffff888079480000(0000) knlGS:0[...]
    [] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    [] CR2: 0000000000000000 CR3: 0000000077320000 CR4: 00000000000006e0
    [] Call Trace:
    []  ? tipc_bcast_rcv+0x9a/0x1a0 [tipc]
    []  tipc_rcv+0x40d/0x670 [tipc]
    []  ? _raw_spin_unlock+0xa/0x20
    []  tipc_l2_rcv_msg+0x55/0x80 [tipc]
    []  __netif_receive_skb_one_core+0x8c/0xa0
    []  process_backlog+0x98/0x140
    []  net_rx_action+0x13a/0x420
    []  __do_softirq+0xdb/0x316
    []  ? smpboot_thread_fn+0x2f/0x1e0
    []  ? smpboot_thread_fn+0x74/0x1e0
    []  ? smpboot_thread_fn+0x14e/0x1e0
    []  run_ksoftirqd+0x1a/0x40
    []  smpboot_thread_fn+0x149/0x1e0
    []  ? sort_range+0x20/0x20
    []  kthread+0x131/0x150
    []  ? kthread_unuse_mm+0xa0/0xa0
    []  ret_from_fork+0x22/0x30
    [] Modules linked in: veth tipc(O) ip6_udp_tunnel udp_tunnel [...]
    [] CR2: 0000000000000000
    [] ---[ end trace 65c276a8e2e2f310 ]---

To fix this, we need to grab the lock of the 'namedq' list on both
path calling.

Fixes: cad2929dc432 ("tipc: update a binding service via broadcast")
Acked-by: Jon Maloy <jm...@re...>
Signed-off-by: Hoang Huu Le <hoa...@de...>
---
 net/tipc/name_distr.c | 10 +++++++++-
 net/tipc/node.c       |  2 +-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
index 2f9c148f17e2..fe4edce459ad 100644
--- a/net/tipc/name_distr.c
+++ b/net/tipc/name_distr.c
@@ -327,8 +327,13 @@ static struct sk_buff *tipc_named_dequeue(struct sk_buff_head *namedq,
 	struct tipc_msg *hdr;
 	u16 seqno;
 
+	spin_lock_bh(&namedq->lock);
 	skb_queue_walk_safe(namedq, skb, tmp) {
-		skb_linearize(skb);
+		if (unlikely(skb_linearize(skb))) {
+			__skb_unlink(skb, namedq);
+			kfree_skb(skb);
+			continue;
+		}
 		hdr = buf_msg(skb);
 		seqno = msg_named_seqno(hdr);
 		if (msg_is_last_bulk(hdr)) {
@@ -338,12 +343,14 @@ static struct sk_buff *tipc_named_dequeue(struct sk_buff_head *namedq,
 
 		if (msg_is_bulk(hdr) || msg_is_legacy(hdr)) {
 			__skb_unlink(skb, namedq);
+			spin_unlock_bh(&namedq->lock);
 			return skb;
 		}
 
 		if (*open && (*rcv_nxt == seqno)) {
 			(*rcv_nxt)++;
 			__skb_unlink(skb, namedq);
+			spin_unlock_bh(&namedq->lock);
 			return skb;
 		}
 
@@ -353,6 +360,7 @@ static struct sk_buff *tipc_named_dequeue(struct sk_buff_head *namedq,
 			continue;
 		}
 	}
+	spin_unlock_bh(&namedq->lock);
 	return NULL;
 }
 
diff --git a/net/tipc/node.c b/net/tipc/node.c
index cf4b239fc569..d269ebe382e1 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -1496,7 +1496,7 @@ static void node_lost_contact(struct tipc_node *n,
 
 	/* Clean up broadcast state */
 	tipc_bcast_remove_peer(n->net, n->bc_entry.link);
-	__skb_queue_purge(&n->bc_entry.namedq);
+	skb_queue_purge(&n->bc_entry.namedq);
 
 	/* Abort any ongoing link failover */
 	for (i = 0; i < MAX_BEARERS; i++) {
-- 
2.25.1

[tipc-discussion] [net-next] tipc: introduce smooth change to replicast

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

In commit cad2929dc432 ("tipc: update a binding service via broadcast"),
We use broadcast to update a binding service for a large cluster.
However, if we try to publish a thousands of services at the
same time, we may to get "link overflow" happen because of queue limit
had reached.

We now introduce a smooth change to replicast if the broadcast link has
reach to the limit of queue.

Signed-off-by: Hoang Huu Le <hoa...@de...>
---
 net/tipc/link.c |  5 ++++-
 net/tipc/node.c | 12 ++++++++++--
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index 06b880da2a8e..ca908ead753a 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -1022,7 +1022,10 @@ int tipc_link_xmit(struct tipc_link *l, struct sk_buff_head *list,
 	/* Allow oversubscription of one data msg per source at congestion */
 	if (unlikely(l->backlog[imp].len >= l->backlog[imp].limit)) {
 		if (imp == TIPC_SYSTEM_IMPORTANCE) {
-			pr_warn("%s<%s>, link overflow", link_rst_msg, l->name);
+			pr_warn_ratelimited("%s<%s>, link overflow",
+					    link_rst_msg, l->name);
+			if (link_is_bc_sndlink(l))
+				return -EOVERFLOW;
 			return -ENOBUFS;
 		}
 		rc = link_schedule_user(l, hdr);
diff --git a/net/tipc/node.c b/net/tipc/node.c
index d269ebe382e1..a37976610367 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -1750,15 +1750,23 @@ void tipc_node_broadcast(struct net *net, struct sk_buff *skb, int rc_dests)
 	struct tipc_node *n;
 	u16 dummy;
 	u32 dst;
+	int rc = 0;
 
 	/* Use broadcast if all nodes support it */
 	if (!rc_dests && tipc_bcast_get_mode(net) != BCLINK_MODE_RCAST) {
+		txskb = pskb_copy(skb, GFP_ATOMIC);
+		if (!txskb)
+			goto rcast;
 		__skb_queue_head_init(&xmitq);
-		__skb_queue_tail(&xmitq, skb);
-		tipc_bcast_xmit(net, &xmitq, &dummy);
+		__skb_queue_tail(&xmitq, txskb);
+		rc = tipc_bcast_xmit(net, &xmitq, &dummy);
+		if (rc == -EOVERFLOW)
+			goto rcast;
+		kfree_skb(skb);
 		return;
 	}
 
+rcast:
 	/* Otherwise use legacy replicast method */
 	rcu_read_lock();
 	list_for_each_entry_rcu(n, tipc_nodes(net), list) {
-- 
2.25.1

Re: [tipc-discussion] [net-next] tipc: fix NULL pointer dereference in tipc_named_rcv

[Jon M. <jm...@re...>]

On 10/7/20 8:13 AM, Hoang Huu Le wrote:
> In the function node_lost_contact(), we call __skb_queue_purge() without
> grabbing the list->lock. This can cause to a race-condition why processing
> the list 'namedq' in calling path tipc_named_rcv()->tipc_named_dequeue().
>
>      [] BUG: kernel NULL pointer dereference, address: 0000000000000000
>      [] #PF: supervisor read access in kernel mode
>      [] #PF: error_code(0x0000) - not-present page
>      [] PGD 7ca63067 P4D 7ca63067 PUD 6c553067 PMD 0
>      [] Oops: 0000 [#1] SMP NOPTI
>      [] CPU: 1 PID: 15 Comm: ksoftirqd/1 Tainted: G  O  5.9.0-rc6+ #2
>      [] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS [...]
>      [] RIP: 0010:tipc_named_rcv+0x103/0x320 [tipc]
>      [] Code: 41 89 44 24 10 49 8b 16 49 8b 46 08 49 c7 06 00 00 00 [...]
>      [] RSP: 0018:ffffc900000a7c58 EFLAGS: 00000282
>      [] RAX: 00000000000012ec RBX: 0000000000000000 RCX: ffff88807bde1270
>      [] RDX: 0000000000002c7c RSI: 0000000000002c7c RDI: ffff88807b38f1a8
>      [] RBP: ffff88807b006288 R08: ffff88806a367800 R09: ffff88806a367900
>      [] R10: ffff88806a367a00 R11: ffff88806a367b00 R12: ffff88807b006258
>      [] R13: ffff88807b00628a R14: ffff888069334d00 R15: ffff88806a434600
>      [] FS:  0000000000000000(0000) GS:ffff888079480000(0000) knlGS:0[...]
>      [] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>      [] CR2: 0000000000000000 CR3: 0000000077320000 CR4: 00000000000006e0
>      [] Call Trace:
>      []  ? tipc_bcast_rcv+0x9a/0x1a0 [tipc]
>      []  tipc_rcv+0x40d/0x670 [tipc]
>      []  ? _raw_spin_unlock+0xa/0x20
>      []  tipc_l2_rcv_msg+0x55/0x80 [tipc]
>      []  __netif_receive_skb_one_core+0x8c/0xa0
>      []  process_backlog+0x98/0x140
>      []  net_rx_action+0x13a/0x420
>      []  __do_softirq+0xdb/0x316
>      []  ? smpboot_thread_fn+0x2f/0x1e0
>      []  ? smpboot_thread_fn+0x74/0x1e0
>      []  ? smpboot_thread_fn+0x14e/0x1e0
>      []  run_ksoftirqd+0x1a/0x40
>      []  smpboot_thread_fn+0x149/0x1e0
>      []  ? sort_range+0x20/0x20
>      []  kthread+0x131/0x150
>      []  ? kthread_unuse_mm+0xa0/0xa0
>      []  ret_from_fork+0x22/0x30
>      [] Modules linked in: veth tipc(O) ip6_udp_tunnel udp_tunnel [...]
>      [] CR2: 0000000000000000
>      [] ---[ end trace 65c276a8e2e2f310 ]---
>
> To fix this, we need to grab the lock of the 'namedq' list on both
> path calling.
>
> Fixes: cad2929dc432 ("tipc: update a binding service via broadcast")
> Signed-off-by: Hoang Huu Le <hoa...@de...>
> ---
>   net/tipc/name_distr.c | 10 +++++++++-
>   net/tipc/node.c       |  2 +-
>   2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
> index 2f9c148f17e2..fe4edce459ad 100644
> --- a/net/tipc/name_distr.c
> +++ b/net/tipc/name_distr.c
> @@ -327,8 +327,13 @@ static struct sk_buff *tipc_named_dequeue(struct sk_buff_head *namedq,
>   	struct tipc_msg *hdr;
>   	u16 seqno;
>   
> +	spin_lock_bh(&namedq->lock);
>   	skb_queue_walk_safe(namedq, skb, tmp) {
> -		skb_linearize(skb);
> +		if (unlikely(skb_linearize(skb))) {
> +			__skb_unlink(skb, namedq);
> +			kfree_skb(skb);
> +			continue;
> +		}
>   		hdr = buf_msg(skb);
>   		seqno = msg_named_seqno(hdr);
>   		if (msg_is_last_bulk(hdr)) {
> @@ -338,12 +343,14 @@ static struct sk_buff *tipc_named_dequeue(struct sk_buff_head *namedq,
>   
>   		if (msg_is_bulk(hdr) || msg_is_legacy(hdr)) {
>   			__skb_unlink(skb, namedq);
> +			spin_unlock_bh(&namedq->lock);
>   			return skb;
>   		}
>   
>   		if (*open && (*rcv_nxt == seqno)) {
>   			(*rcv_nxt)++;
>   			__skb_unlink(skb, namedq);
> +			spin_unlock_bh(&namedq->lock);
>   			return skb;
>   		}
>   
> @@ -353,6 +360,7 @@ static struct sk_buff *tipc_named_dequeue(struct sk_buff_head *namedq,
>   			continue;
>   		}
>   	}
> +	spin_unlock_bh(&namedq->lock);
>   	return NULL;
>   }
>   
> diff --git a/net/tipc/node.c b/net/tipc/node.c
> index cf4b239fc569..d269ebe382e1 100644
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -1496,7 +1496,7 @@ static void node_lost_contact(struct tipc_node *n,
>   
>   	/* Clean up broadcast state */
>   	tipc_bcast_remove_peer(n->net, n->bc_entry.link);
> -	__skb_queue_purge(&n->bc_entry.namedq);
> +	skb_queue_purge(&n->bc_entry.namedq);
>   
>   	/* Abort any ongoing link failover */
>   	for (i = 0; i < MAX_BEARERS; i++) {
Looks ok.
Acked-by: jon Maloy <jm...@re...>

[tipc-discussion] [net-next] tipc: fix NULL pointer dereference in tipc_named_rcv

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

In the function node_lost_contact(), we call __skb_queue_purge() without
grabbing the list->lock. This can cause to a race-condition why processing
the list 'namedq' in calling path tipc_named_rcv()->tipc_named_dequeue().

    [] BUG: kernel NULL pointer dereference, address: 0000000000000000
    [] #PF: supervisor read access in kernel mode
    [] #PF: error_code(0x0000) - not-present page
    [] PGD 7ca63067 P4D 7ca63067 PUD 6c553067 PMD 0
    [] Oops: 0000 [#1] SMP NOPTI
    [] CPU: 1 PID: 15 Comm: ksoftirqd/1 Tainted: G  O  5.9.0-rc6+ #2
    [] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS [...]
    [] RIP: 0010:tipc_named_rcv+0x103/0x320 [tipc]
    [] Code: 41 89 44 24 10 49 8b 16 49 8b 46 08 49 c7 06 00 00 00 [...]
    [] RSP: 0018:ffffc900000a7c58 EFLAGS: 00000282
    [] RAX: 00000000000012ec RBX: 0000000000000000 RCX: ffff88807bde1270
    [] RDX: 0000000000002c7c RSI: 0000000000002c7c RDI: ffff88807b38f1a8
    [] RBP: ffff88807b006288 R08: ffff88806a367800 R09: ffff88806a367900
    [] R10: ffff88806a367a00 R11: ffff88806a367b00 R12: ffff88807b006258
    [] R13: ffff88807b00628a R14: ffff888069334d00 R15: ffff88806a434600
    [] FS:  0000000000000000(0000) GS:ffff888079480000(0000) knlGS:0[...]
    [] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    [] CR2: 0000000000000000 CR3: 0000000077320000 CR4: 00000000000006e0
    [] Call Trace:
    []  ? tipc_bcast_rcv+0x9a/0x1a0 [tipc]
    []  tipc_rcv+0x40d/0x670 [tipc]
    []  ? _raw_spin_unlock+0xa/0x20
    []  tipc_l2_rcv_msg+0x55/0x80 [tipc]
    []  __netif_receive_skb_one_core+0x8c/0xa0
    []  process_backlog+0x98/0x140
    []  net_rx_action+0x13a/0x420
    []  __do_softirq+0xdb/0x316
    []  ? smpboot_thread_fn+0x2f/0x1e0
    []  ? smpboot_thread_fn+0x74/0x1e0
    []  ? smpboot_thread_fn+0x14e/0x1e0
    []  run_ksoftirqd+0x1a/0x40
    []  smpboot_thread_fn+0x149/0x1e0
    []  ? sort_range+0x20/0x20
    []  kthread+0x131/0x150
    []  ? kthread_unuse_mm+0xa0/0xa0
    []  ret_from_fork+0x22/0x30
    [] Modules linked in: veth tipc(O) ip6_udp_tunnel udp_tunnel [...]
    [] CR2: 0000000000000000
    [] ---[ end trace 65c276a8e2e2f310 ]---

To fix this, we need to grab the lock of the 'namedq' list on both
path calling.

Fixes: cad2929dc432 ("tipc: update a binding service via broadcast")
Signed-off-by: Hoang Huu Le <hoa...@de...>
---
 net/tipc/name_distr.c | 10 +++++++++-
 net/tipc/node.c       |  2 +-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
index 2f9c148f17e2..fe4edce459ad 100644
--- a/net/tipc/name_distr.c
+++ b/net/tipc/name_distr.c
@@ -327,8 +327,13 @@ static struct sk_buff *tipc_named_dequeue(struct sk_buff_head *namedq,
 	struct tipc_msg *hdr;
 	u16 seqno;
 
+	spin_lock_bh(&namedq->lock);
 	skb_queue_walk_safe(namedq, skb, tmp) {
-		skb_linearize(skb);
+		if (unlikely(skb_linearize(skb))) {
+			__skb_unlink(skb, namedq);
+			kfree_skb(skb);
+			continue;
+		}
 		hdr = buf_msg(skb);
 		seqno = msg_named_seqno(hdr);
 		if (msg_is_last_bulk(hdr)) {
@@ -338,12 +343,14 @@ static struct sk_buff *tipc_named_dequeue(struct sk_buff_head *namedq,
 
 		if (msg_is_bulk(hdr) || msg_is_legacy(hdr)) {
 			__skb_unlink(skb, namedq);
+			spin_unlock_bh(&namedq->lock);
 			return skb;
 		}
 
 		if (*open && (*rcv_nxt == seqno)) {
 			(*rcv_nxt)++;
 			__skb_unlink(skb, namedq);
+			spin_unlock_bh(&namedq->lock);
 			return skb;
 		}
 
@@ -353,6 +360,7 @@ static struct sk_buff *tipc_named_dequeue(struct sk_buff_head *namedq,
 			continue;
 		}
 	}
+	spin_unlock_bh(&namedq->lock);
 	return NULL;
 }
 
diff --git a/net/tipc/node.c b/net/tipc/node.c
index cf4b239fc569..d269ebe382e1 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -1496,7 +1496,7 @@ static void node_lost_contact(struct tipc_node *n,
 
 	/* Clean up broadcast state */
 	tipc_bcast_remove_peer(n->net, n->bc_entry.link);
-	__skb_queue_purge(&n->bc_entry.namedq);
+	skb_queue_purge(&n->bc_entry.namedq);
 
 	/* Abort any ongoing link failover */
 	for (i = 0; i < MAX_BEARERS; i++) {
-- 
2.25.1

Re: [tipc-discussion] tipc.py

[Jon M. <jm...@re...>]

On 10/5/20 12:35 AM, Hoang Huu Le wrote:
> Hi Jon,
>
> I will make an effort on this. Do you think we need to keep these APIs compatibility work with Python2 or completely remove out?
Great! To me you can just update the current code and don't worry about 
compatibility.
If anybody has used the current module they will have their own copy, 
and I cannot imagine that anybody will write new programs for Puthon 2 
by now.

Regards
///jon

>
> Regards,
> Hoang
>> -----Original Message-----
>> From: Jon Maloy <jm...@re...>
>> Sent: Saturday, October 3, 2020 5:59 AM
>> To: Hoang Huu Le <hoa...@de...>; tip...@li...; tipc-dek <tip...@de...>; Xin
>> Long <luc...@gm...>; Ying Xue <yin...@wi...>
>> Subject: Re: tipc.py
>>
>>
>>
>> On 10/1/20 11:04 PM, Hoang Huu Le wrote:
>>> Hi Jon,
>>>
>>> I've done this a long time ago:
>>> 5057f8bb4de0 tipcutils: introduce python api
>>>
>>> Basically, it works with Python 2.
>> Do you think it you would have time to do this for Python 3?
>> Python 2 is practically dead now, as we all know.
>>
>> Regards
>> ///jon
>>> Regards,
>>> Hoang
>>>> -----Original Message-----
>>>> From: Jon Maloy <jm...@re...>
>>>> Sent: Friday, October 2, 2020 4:56 AM
>>>> To: tip...@li...; tipc-dek <tip...@de...>; Xin Long <luc...@gm...>; Ying Xue
>>>> <yin...@wi...>
>>>> Subject: tipc.py
>>>>
>>>> I am updating the programmer's manual, and realized that the Python API
>>>> is missing.
>>>> Since there are so many programmers knowing Python nowadays, but not C,
>>>> I think it would
>>>> be very useful to have this API in the manual, so those programmers can
>>>> get a feeling
>>>> for how simple it to use TIPC.
>>>>
>>>> Tuong started development of an API based on the libtipc C-API, but it
>>>> seems to me it was never finished.
>>>> However, Python does since a long time have native TIPC support,
>>>> allegedly both in Python 2 and Pyton 3.
>>>> I had never seen that API until now, but after some googling I came over
>>>> the following link, that seems to contain
>>>> that native implemenation:
>>>>
>>>> https://blitiri.com.ar/p/pytipc/
>>>>
>>>> I wonder if anybody has the time to try this one, and verify, using the
>>>> examples, that it really works.
>>>> It would be embarrassing to add this to the manual if it turns out it
>>>> doesn't work.
>>>>
>>>> Regards
>>>> ///jon
>>>>
>>>> PS. Does anybody volunteer to be become co-maintainer of the home page
>>>> and project page
>>>>          at SourceForge? I think we should even consider moving it to
>>>> GitLab or GitHub.
>>>>          Since we have our own domain (tipc.io) that could easily be
>>>> re-steered to a different
>>>>          host system.

[tipc-discussion] [net v1 1/1] tipc: Fix memory leak in tipc_link_xmit

[Tung N. <tun...@de...>]

In case the backlog transmit queue for system-importance messages is
overloaded, tipc_link_xmit() returns -ENOBUFS but the skb list is not
purged. This leads to memory leak and failure when a skb is allocated.

This commit fixes this issue by purging the skb list before
tipc_link_xmit() returns.

Reported-by: Thang Hoang Ngo <tha...@de...>
Signed-off-by: Tung Nguyen <tun...@de...>
---
 net/tipc/link.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index cef38a910107..ca0bb09482d0 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -1028,6 +1028,7 @@ int tipc_link_xmit(struct tipc_link *l, struct sk_buff_head *list,
 	if (unlikely(l->backlog[imp].len >= l->backlog[imp].limit)) {
 		if (imp == TIPC_SYSTEM_IMPORTANCE) {
 			pr_warn("%s<%s>, link overflow", link_rst_msg, l->name);
+			__skb_queue_purge(list);
 			return -ENOBUFS;
 		}
 		rc = link_schedule_user(l, hdr);
-- 
2.17.1

Re: [tipc-discussion] tipc.py

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

Hi Jon,

I will make an effort on this. Do you think we need to keep these APIs compatibility work with Python2 or completely remove out?

Regards,
Hoang
> -----Original Message-----
> From: Jon Maloy <jm...@re...>
> Sent: Saturday, October 3, 2020 5:59 AM
> To: Hoang Huu Le <hoa...@de...>; tip...@li...; tipc-dek <tip...@de...>; Xin
> Long <luc...@gm...>; Ying Xue <yin...@wi...>
> Subject: Re: tipc.py
> 
> 
> 
> On 10/1/20 11:04 PM, Hoang Huu Le wrote:
> > Hi Jon,
> >
> > I've done this a long time ago:
> > 5057f8bb4de0 tipcutils: introduce python api
> >
> > Basically, it works with Python 2.
> Do you think it you would have time to do this for Python 3?
> Python 2 is practically dead now, as we all know.
> 
> Regards
> ///jon
> >
> > Regards,
> > Hoang
> >> -----Original Message-----
> >> From: Jon Maloy <jm...@re...>
> >> Sent: Friday, October 2, 2020 4:56 AM
> >> To: tip...@li...; tipc-dek <tip...@de...>; Xin Long <luc...@gm...>; Ying Xue
> >> <yin...@wi...>
> >> Subject: tipc.py
> >>
> >> I am updating the programmer's manual, and realized that the Python API
> >> is missing.
> >> Since there are so many programmers knowing Python nowadays, but not C,
> >> I think it would
> >> be very useful to have this API in the manual, so those programmers can
> >> get a feeling
> >> for how simple it to use TIPC.
> >>
> >> Tuong started development of an API based on the libtipc C-API, but it
> >> seems to me it was never finished.
> >> However, Python does since a long time have native TIPC support,
> >> allegedly both in Python 2 and Pyton 3.
> >> I had never seen that API until now, but after some googling I came over
> >> the following link, that seems to contain
> >> that native implemenation:
> >>
> >> https://blitiri.com.ar/p/pytipc/
> >>
> >> I wonder if anybody has the time to try this one, and verify, using the
> >> examples, that it really works.
> >> It would be embarrassing to add this to the manual if it turns out it
> >> doesn't work.
> >>
> >> Regards
> >> ///jon
> >>
> >> PS. Does anybody volunteer to be become co-maintainer of the home page
> >> and project page
> >>         at SourceForge? I think we should even consider moving it to
> >> GitLab or GitHub.
> >>         Since we have our own domain (tipc.io) that could easily be
> >> re-steered to a different
> >>         host system.

Re: [tipc-discussion] KASAN: use-after-free Read in tipc_mcast_xmit (2)

[Xin L. <luc...@gm...>]

On Fri, Oct 2, 2020 at 11:38 PM syzbot
<syz...@sy...> wrote:
>
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit:    a59cf619 Merge branch 'Fix-bugs-in-Octeontx2-netdev-driver'
> git tree:       bpf
> console output: https://syzkaller.appspot.com/x/log.txt?x=163c2467900000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=99a7c78965c75e07
> dashboard link: https://syzkaller.appspot.com/bug?extid=e96a7ba46281824cc46a
> compiler:       gcc (GCC) 10.1.0-syz 20200507
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=15ada44d900000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=14007467900000
>
> The issue was bisected to:
>
> commit ff48b6222e65ebdba5a403ef1deba6214e749193
> Author: Xin Long <luc...@gm...>
> Date:   Sun Sep 13 11:37:31 2020 +0000
>
>     tipc: use skb_unshare() instead in tipc_buf_append()
>
> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=125402b3900000
> final oops:     https://syzkaller.appspot.com/x/report.txt?x=115402b3900000
> console output: https://syzkaller.appspot.com/x/log.txt?x=165402b3900000
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syz...@sy...
> Fixes: ff48b6222e65 ("tipc: use skb_unshare() instead in tipc_buf_append()")
>
> R10: 0000000000000000 R11: 0000000000000246 R12: 00000000004028a0
> R13: 0000000000402930 R14: 0000000000000000 R15: 0000000000000000
> tipc: Failed do clone local mcast rcv buffer
> ==================================================================
> BUG: KASAN: use-after-free in __skb_unlink include/linux/skbuff.h:2063 [inline]
> BUG: KASAN: use-after-free in __skb_dequeue include/linux/skbuff.h:2082 [inline]
> BUG: KASAN: use-after-free in __skb_queue_purge include/linux/skbuff.h:2793 [inline]
> BUG: KASAN: use-after-free in tipc_mcast_xmit+0xfaa/0x1170 net/tipc/bcast.c:422
> Read of size 8 at addr ffff8880a73e2040 by task syz-executor657/6887
>
> CPU: 1 PID: 6887 Comm: syz-executor657 Not tainted 5.9.0-rc6-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
>  __dump_stack lib/dump_stack.c:77 [inline]
>  dump_stack+0x198/0x1fd lib/dump_stack.c:118
>  print_address_description.constprop.0.cold+0xae/0x497 mm/kasan/report.c:383
>  __kasan_report mm/kasan/report.c:513 [inline]
>  kasan_report.cold+0x1f/0x37 mm/kasan/report.c:530
>  __skb_unlink include/linux/skbuff.h:2063 [inline]
>  __skb_dequeue include/linux/skbuff.h:2082 [inline]
>  __skb_queue_purge include/linux/skbuff.h:2793 [inline]
>  tipc_mcast_xmit+0xfaa/0x1170 net/tipc/bcast.c:422
>  tipc_sendmcast+0xaaf/0xef0 net/tipc/socket.c:865
>  __tipc_sendmsg+0xee3/0x18a0 net/tipc/socket.c:1454
>  tipc_sendmsg+0x4c/0x70 net/tipc/socket.c:1387
>  sock_sendmsg_nosec net/socket.c:651 [inline]
>  sock_sendmsg+0xcf/0x120 net/socket.c:671
>  ____sys_sendmsg+0x6e8/0x810 net/socket.c:2353
>  ___sys_sendmsg+0xf3/0x170 net/socket.c:2407
>  __sys_sendmsg+0xe5/0x1b0 net/socket.c:2440
>  do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x4419d9
> Code: e8 cc ac 02 00 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 3b 0a fc ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:00007ffe0cace4c8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00000000004419d9
> RDX: 0000000000000000 RSI: 0000000020000280 RDI: 0000000000000004
> RBP: 000000000000f0ee R08: 0000000000000001 R09: 0000000000402930
> R10: 0000000000000000 R11: 0000000000000246 R12: 00000000004028a0
> R13: 0000000000402930 R14: 0000000000000000 R15: 0000000000000000
>
> Allocated by task 6887:
>  kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
>  kasan_set_track mm/kasan/common.c:56 [inline]
>  __kasan_kmalloc.constprop.0+0xbf/0xd0 mm/kasan/common.c:461
>  slab_post_alloc_hook mm/slab.h:518 [inline]
>  slab_alloc_node mm/slab.c:3254 [inline]
>  kmem_cache_alloc_node+0x136/0x430 mm/slab.c:3574
>  __alloc_skb+0x71/0x550 net/core/skbuff.c:198
>  alloc_skb_fclone include/linux/skbuff.h:1144 [inline]
>  tipc_buf_acquire+0x28/0xf0 net/tipc/msg.c:76
>  tipc_msg_build+0x6b8/0x10c0 net/tipc/msg.c:428
>  tipc_sendmcast+0x855/0xef0 net/tipc/socket.c:859
>  __tipc_sendmsg+0xee3/0x18a0 net/tipc/socket.c:1454
>  tipc_sendmsg+0x4c/0x70 net/tipc/socket.c:1387
>  sock_sendmsg_nosec net/socket.c:651 [inline]
>  sock_sendmsg+0xcf/0x120 net/socket.c:671
>  ____sys_sendmsg+0x6e8/0x810 net/socket.c:2353
>  ___sys_sendmsg+0xf3/0x170 net/socket.c:2407
>  __sys_sendmsg+0xe5/0x1b0 net/socket.c:2440
>  do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> Freed by task 6887:
>  kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
>  kasan_set_track+0x1c/0x30 mm/kasan/common.c:56
>  kasan_set_free_info+0x1b/0x30 mm/kasan/generic.c:355
>  __kasan_slab_free+0xd8/0x120 mm/kasan/common.c:422
>  __cache_free mm/slab.c:3418 [inline]
>  kmem_cache_free.part.0+0x74/0x1e0 mm/slab.c:3693
>  kfree_skbmem+0x166/0x1b0 net/core/skbuff.c:643
>  kfree_skb+0x7d/0x100 include/linux/refcount.h:270
>  tipc_buf_append+0x6dc/0xcf0 net/tipc/msg.c:198
>  tipc_msg_reassemble+0x175/0x4f0 net/tipc/msg.c:790
>  tipc_mcast_xmit+0x699/0x1170 net/tipc/bcast.c:386
>  tipc_sendmcast+0xaaf/0xef0 net/tipc/socket.c:865
>  __tipc_sendmsg+0xee3/0x18a0 net/tipc/socket.c:1454
>  tipc_sendmsg+0x4c/0x70 net/tipc/socket.c:1387
>  sock_sendmsg_nosec net/socket.c:651 [inline]
>  sock_sendmsg+0xcf/0x120 net/socket.c:671
>  ____sys_sendmsg+0x6e8/0x810 net/socket.c:2353
>  ___sys_sendmsg+0xf3/0x170 net/socket.c:2407
>  __sys_sendmsg+0xe5/0x1b0 net/socket.c:2440
>  do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
in tipc_msg_reassemble():

                if ((&head, &frag))
                        break;
                if (!head)
                        goto error; <--- [1]
        }
        __skb_queue_tail(rcvq, frag);
        return true;
error:
        pr_warn("Failed do clone local mcast rcv buffer\n");
        kfree_skb(head); <---[2]
        return false;

when head is NULL at [1], it goes [2] and could cause a crash.
from the log, we can see "Failed do clone local mcast rcv buffer" as well.

will check and make a fix for this.

Thanks.

>
> The buggy address belongs to the object at ffff8880a73e2040
>  which belongs to the cache skbuff_fclone_cache of size 456
> The buggy address is located 0 bytes inside of
>  456-byte region [ffff8880a73e2040, ffff8880a73e2208)
> The buggy address belongs to the page:
> page:000000001368f319 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0xa73e2
> flags: 0xfffe0000000200(slab)
> raw: 00fffe0000000200 ffff8880a9050f50 ffffea00028ff188 ffff8880a903dc00
> raw: 0000000000000000 ffff8880a73e2040 0000000100000006 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
>  ffff8880a73e1f00: fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc fc
>  ffff8880a73e1f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >ffff8880a73e2000: fc fc fc fc fc fc fc fc fa fb fb fb fb fb fb fb
>                                            ^
>  ffff8880a73e2080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>  ffff8880a73e2100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ==================================================================
>
>
> ---
> This report is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syz...@go....
>
> syzbot will keep track of this issue. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
> For information about bisection process see: https://goo.gl/tpsmEJ#bisection
> syzbot can test patches for this issue, for details see:
> https://goo.gl/tpsmEJ#testing-patches

Re: [tipc-discussion] tipc.py

[Jon M. <jm...@re...>]

On 10/1/20 11:04 PM, Hoang Huu Le wrote:
> Hi Jon,
>
> I've done this a long time ago:
> 5057f8bb4de0 tipcutils: introduce python api
>
> Basically, it works with Python 2.
Do you think it you would have time to do this for Python 3?
Python 2 is practically dead now, as we all know.

Regards
///jon
>
> Regards,
> Hoang
>> -----Original Message-----
>> From: Jon Maloy <jm...@re...>
>> Sent: Friday, October 2, 2020 4:56 AM
>> To: tip...@li...; tipc-dek <tip...@de...>; Xin Long <luc...@gm...>; Ying Xue
>> <yin...@wi...>
>> Subject: tipc.py
>>
>> I am updating the programmer's manual, and realized that the Python API
>> is missing.
>> Since there are so many programmers knowing Python nowadays, but not C,
>> I think it would
>> be very useful to have this API in the manual, so those programmers can
>> get a feeling
>> for how simple it to use TIPC.
>>
>> Tuong started development of an API based on the libtipc C-API, but it
>> seems to me it was never finished.
>> However, Python does since a long time have native TIPC support,
>> allegedly both in Python 2 and Pyton 3.
>> I had never seen that API until now, but after some googling I came over
>> the following link, that seems to contain
>> that native implemenation:
>>
>> https://blitiri.com.ar/p/pytipc/
>>
>> I wonder if anybody has the time to try this one, and verify, using the
>> examples, that it really works.
>> It would be embarrassing to add this to the manual if it turns out it
>> doesn't work.
>>
>> Regards
>> ///jon
>>
>> PS. Does anybody volunteer to be become co-maintainer of the home page
>> and project page
>>         at SourceForge? I think we should even consider moving it to
>> GitLab or GitHub.
>>         Since we have our own domain (tipc.io) that could easily be
>> re-steered to a different
>>         host system.

Re: [tipc-discussion] tipc.py

[Jon M. <jm...@re...>]

On 10/1/20 11:04 PM, Hoang Huu Le wrote:
> Hi Jon,
>
> I've done this a long time ago:
> 5057f8bb4de0 tipcutils: introduce python api
>
> Basically, it works with Python 2.
>
> Regards,
> Hoang
Sorry, I mixed it up and thought it was Tuong. The reason I said it 
looked "unfinished" was all the "TODO:" lines I see in tipc.py.
What are those?

///jon

>> -----Original Message-----
>> From: Jon Maloy <jm...@re...>
>> Sent: Friday, October 2, 2020 4:56 AM
>> To: tip...@li...; tipc-dek <tip...@de...>; Xin Long <luc...@gm...>; Ying Xue
>> <yin...@wi...>
>> Subject: tipc.py
>>
>> I am updating the programmer's manual, and realized that the Python API
>> is missing.
>> Since there are so many programmers knowing Python nowadays, but not C,
>> I think it would
>> be very useful to have this API in the manual, so those programmers can
>> get a feeling
>> for how simple it to use TIPC.
>>
>> Tuong started development of an API based on the libtipc C-API, but it
>> seems to me it was never finished.
>> However, Python does since a long time have native TIPC support,
>> allegedly both in Python 2 and Pyton 3.
>> I had never seen that API until now, but after some googling I came over
>> the following link, that seems to contain
>> that native implemenation:
>>
>> https://blitiri.com.ar/p/pytipc/
>>
>> I wonder if anybody has the time to try this one, and verify, using the
>> examples, that it really works.
>> It would be embarrassing to add this to the manual if it turns out it
>> doesn't work.
>>
>> Regards
>> ///jon
>>
>> PS. Does anybody volunteer to be become co-maintainer of the home page
>> and project page
>>         at SourceForge? I think we should even consider moving it to
>> GitLab or GitHub.
>>         Since we have our own domain (tipc.io) that could easily be
>> re-steered to a different
>>         host system.

Re: [tipc-discussion] tipc.py

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

Hi Jon,

I've done this a long time ago:
5057f8bb4de0 tipcutils: introduce python api

Basically, it works with Python 2.

Regards,
Hoang
> -----Original Message-----
> From: Jon Maloy <jm...@re...>
> Sent: Friday, October 2, 2020 4:56 AM
> To: tip...@li...; tipc-dek <tip...@de...>; Xin Long <luc...@gm...>; Ying Xue
> <yin...@wi...>
> Subject: tipc.py
> 
> I am updating the programmer's manual, and realized that the Python API
> is missing.
> Since there are so many programmers knowing Python nowadays, but not C,
> I think it would
> be very useful to have this API in the manual, so those programmers can
> get a feeling
> for how simple it to use TIPC.
> 
> Tuong started development of an API based on the libtipc C-API, but it
> seems to me it was never finished.
> However, Python does since a long time have native TIPC support,
> allegedly both in Python 2 and Pyton 3.
> I had never seen that API until now, but after some googling I came over
> the following link, that seems to contain
> that native implemenation:
> 
> https://blitiri.com.ar/p/pytipc/
> 
> I wonder if anybody has the time to try this one, and verify, using the
> examples, that it really works.
> It would be embarrassing to add this to the manual if it turns out it
> doesn't work.
> 
> Regards
> ///jon
> 
> PS. Does anybody volunteer to be become co-maintainer of the home page
> and project page
>        at SourceForge? I think we should even consider moving it to
> GitLab or GitHub.
>        Since we have our own domain (tipc.io) that could easily be
> re-steered to a different
>        host system.

Re: [tipc-discussion] [net] tipc: fix incorrect setting window for bcast link

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

Hi Jon,

See my inline comment.

Regards,
Hoang
> -----Original Message-----
> From: Jon Maloy <jm...@re...>
> Sent: Friday, October 2, 2020 4:40 AM
> To: Hoang Huu Le <hoa...@de...>; tip...@li...; ma...@do...;
> yin...@wi...; Xin Long <luc...@gm...>
> Subject: Re: [net] tipc: fix incorrect setting window for bcast link
> 
> 
> 
> On 9/30/20 9:43 PM, Hoang Huu Le wrote:
> > In commit 16ad3f4022bb
> > ("tipc: introduce variable window congestion control"), we applied
> > the Reno algorithm to select window size from minimum window to the
> > configured maximum window for unicast link.
> >
> > However, when setting window variable we do not specific to unicast link.
> > This lead to the window for broadcast link had effect as unexpect value
> > (i.e the window size is constantly 32).
> This was intentional, although I thought the value was 50, not 32.
> In my experience we cannot afford a generous variable window
> in the broadcast link the same way we do with the unicast link.
> There will be too many losses and retransmissions because of the
> way switches work.

[Hoang]
When it is created, the value is 50. However, if we use the tipc command:
i.e $ tipc link set win 50 link broadcast
The window is set to 32 - this value is constant since then. 
Is it counting as bug?

> 
> >
> > We fix this by updating the window for broadcast as its configuration.
> >
> > Signed-off-by: Hoang Huu Le <hoa...@de...>
> > ---
> >   net/tipc/bcast.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
> > index 940d176e0e87..abac9443b4d9 100644
> > --- a/net/tipc/bcast.c
> > +++ b/net/tipc/bcast.c
> > @@ -585,7 +585,7 @@ static int tipc_bc_link_set_queue_limits(struct net *net, u32 max_win)
> >   	if (max_win > TIPC_MAX_LINK_WIN)
> >   		return -EINVAL;
> >   	tipc_bcast_lock(net);
> > -	tipc_link_set_queue_limits(l, BCLINK_WIN_MIN, max_win);
> > +	tipc_link_set_queue_limits(l, max_win, max_win);
> >   	tipc_bcast_unlock(net);
> >   	return 0;
> >   }
> What is the effect of this change? Do we still have a fix window?
[Hoang]
No, now window can be configured as user intention.
If you'd like to use the fix window 50. I will update the code change. 

> What happens when we have buffer overflow? The broadcast
> send link can never be reset I rember correctly.
> Did you test this with high load, e.g. using the multicast_blast test
> program?
[Hoang] I tried to publish hundreds or services that uses SYSTEM_IMPORTANCE - it was not being reset when the link is overflow.
For others imp, the overflow issue will not happen because of oversubscription allowing. I will give a try with the test.
> 
> Regards
> ///jon

[tipc-discussion] tipc.py

[Jon M. <jm...@re...>]

I am updating the programmer's manual, and realized that the Python API 
is missing.
Since there are so many programmers knowing Python nowadays, but not C, 
I think it would
be very useful to have this API in the manual, so those programmers can 
get a feeling
for how simple it to use TIPC.

Tuong started development of an API based on the libtipc C-API, but it 
seems to me it was never finished.
However, Python does since a long time have native TIPC support, 
allegedly both in Python 2 and Pyton 3.
I had never seen that API until now, but after some googling I came over 
the following link, that seems to contain
that native implemenation:

https://blitiri.com.ar/p/pytipc/

I wonder if anybody has the time to try this one, and verify, using the 
examples, that it really works.
It would be embarrassing to add this to the manual if it turns out it 
doesn't work.

Regards
///jon

PS. Does anybody volunteer to be become co-maintainer of the home page 
and project page
       at SourceForge? I think we should even consider moving it to 
GitLab or GitHub.
       Since we have our own domain (tipc.io) that could easily be 
re-steered to a different
       host system.

Re: [tipc-discussion] [net] tipc: fix incorrect setting window for bcast link

[Jon M. <jm...@re...>]

On 9/30/20 9:43 PM, Hoang Huu Le wrote:
> In commit 16ad3f4022bb
> ("tipc: introduce variable window congestion control"), we applied
> the Reno algorithm to select window size from minimum window to the
> configured maximum window for unicast link.
>
> However, when setting window variable we do not specific to unicast link.
> This lead to the window for broadcast link had effect as unexpect value
> (i.e the window size is constantly 32).
This was intentional, although I thought the value was 50, not 32.
In my experience we cannot afford a generous variable window
in the broadcast link the same way we do with the unicast link.
There will be too many losses and retransmissions because of the
way switches work.

>
> We fix this by updating the window for broadcast as its configuration.
>
> Signed-off-by: Hoang Huu Le <hoa...@de...>
> ---
>   net/tipc/bcast.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
> index 940d176e0e87..abac9443b4d9 100644
> --- a/net/tipc/bcast.c
> +++ b/net/tipc/bcast.c
> @@ -585,7 +585,7 @@ static int tipc_bc_link_set_queue_limits(struct net *net, u32 max_win)
>   	if (max_win > TIPC_MAX_LINK_WIN)
>   		return -EINVAL;
>   	tipc_bcast_lock(net);
> -	tipc_link_set_queue_limits(l, BCLINK_WIN_MIN, max_win);
> +	tipc_link_set_queue_limits(l, max_win, max_win);
>   	tipc_bcast_unlock(net);
>   	return 0;
>   }
What is the effect of this change? Do we still have a fix window?
What happens when we have buffer overflow? The broadcast
send link can never be reset I rember correctly.
Did you test this with high load, e.g. using the multicast_blast test
program?

Regards
///jon

[tipc-discussion] [net-next] tipc: re-configure queue limit for broadcast link

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

The queue limit of the broadcast link is being calculated base on initial
MTU. However, when MTU value changed (e.g manual changing MTU on NIC device,
MTU negotiation etc.,) we do not re-calculate queue limit. This gives
throughput does not reflect with the change.

So fix it by calling the function to re-calculate queue limit of the
broadcast link.

Signed-off-by: Hoang Huu Le <hoa...@de...>
---
 net/tipc/bcast.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
index abac9443b4d9..bc566b304571 100644
--- a/net/tipc/bcast.c
+++ b/net/tipc/bcast.c
@@ -108,6 +108,7 @@ static void tipc_bcbase_select_primary(struct net *net)
 {
 	struct tipc_bc_base *bb = tipc_bc_base(net);
 	int all_dests =  tipc_link_bc_peers(bb->link);
+	int max_win = tipc_link_max_win(bb->link);
 	int i, mtu, prim;
 
 	bb->primary_bearer = INVALID_BEARER_ID;
@@ -121,8 +122,11 @@ static void tipc_bcbase_select_primary(struct net *net)
 			continue;
 
 		mtu = tipc_bearer_mtu(net, i);
-		if (mtu < tipc_link_mtu(bb->link))
+		if (mtu < tipc_link_mtu(bb->link)) {
 			tipc_link_set_mtu(bb->link, mtu);
+			tipc_link_set_queue_limits(bb->link, max_win,
+						   max_win);
+		}
 		bb->bcast_support &= tipc_bearer_bcast_support(net, i);
 		if (bb->dests[i] < all_dests)
 			continue;
-- 
2.25.1

[tipc-discussion] [net] tipc: fix incorrect setting window for bcast link

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

In commit 16ad3f4022bb
("tipc: introduce variable window congestion control"), we applied
the Reno algorithm to select window size from minimum window to the
configured maximum window for unicast link.

However, when setting window variable we do not specific to unicast link.
This lead to the window for broadcast link had effect as unexpect value
(i.e the window size is constantly 32).

We fix this by updating the window for broadcast as its configuration.

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

diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
index 940d176e0e87..abac9443b4d9 100644
--- a/net/tipc/bcast.c
+++ b/net/tipc/bcast.c
@@ -585,7 +585,7 @@ static int tipc_bc_link_set_queue_limits(struct net *net, u32 max_win)
 	if (max_win > TIPC_MAX_LINK_WIN)
 		return -EINVAL;
 	tipc_bcast_lock(net);
-	tipc_link_set_queue_limits(l, BCLINK_WIN_MIN, max_win);
+	tipc_link_set_queue_limits(l, max_win, max_win);
 	tipc_bcast_unlock(net);
 	return 0;
 }
-- 
2.25.1

Re: [tipc-discussion] [PATCH net-next] tipc: Remove unused macro CF_SERVER

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

From: YueHaibing <yue...@hu...>
Date: Fri, 18 Sep 2020 21:16:15 +0800

> It is no used any more, so can remove it.
> 
> Signed-off-by: YueHaibing <yue...@hu...>

Applied.