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

Re: [tipc-discussion] [net] Revert "net:tipc: Fix a double free in tipc_sk_mcast_rcv"

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

On Fri, May 14, 2021 at 7:18 PM Jon Maloy <jm...@re...> wrote:
>
>
>
> On 5/14/21 6:10 PM, pat...@ke... wrote:
> > Hello:
> >
> > This patch was applied to netdev/net.git (refs/heads/master):
> >
> > On Fri, 14 May 2021 08:23:03 +0700 you wrote:
> >> This reverts commit 6bf24dc0cc0cc43b29ba344b66d78590e687e046.
> >> Above fix is not correct and caused memory leak issue.
>
> I just convinced Xin (and myself) that the crash (double free) he was
> observing, and which he wanted to fix with the "tipc: fix a race in
> tipc_sk_mcast_rcv" patch was due to this bug.
> Now, realizing that this is causing a memory leak and not a double free
> I suspect there might still be an issue.
> Does anybody have a theory?
Hi Jon, I think the double free issue was due to the one I fixed in the patch
I posted:

[PATCH net] tipc: skb_linearize the head skb when reassembling msgs

see the changelog.
>
> ///jon
>
> >>
> >> Fixes: 6bf24dc0cc0c ("net:tipc: Fix a double free in tipc_sk_mcast_rcv")
> >> Acked-by: Jon Maloy <jm...@re...>
> >> Acked-by: Tung Nguyen <tun...@de...>
> >> Signed-off-by: Hoang Le <hoa...@de...>
> >>
> >> [...]
> > Here is the summary with links:
> >    - [net] Revert "net:tipc: Fix a double free in tipc_sk_mcast_rcv"
> >      https://git.kernel.org/netdev/net/c/75016891357a
> >
> > You are awesome, thank you!
> > --
> > Deet-doot-dot, I am a bot.
> > https://korg.docs.kernel.org/patchwork/pwbot.html
> >
> >
>

Re: [tipc-discussion] [net] Revert "net:tipc: Fix a double free in tipc_sk_mcast_rcv"

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

On 5/14/21 6:10 PM, pat...@ke... wrote:
> Hello:
>
> This patch was applied to netdev/net.git (refs/heads/master):
>
> On Fri, 14 May 2021 08:23:03 +0700 you wrote:
>> This reverts commit 6bf24dc0cc0cc43b29ba344b66d78590e687e046.
>> Above fix is not correct and caused memory leak issue.

I just convinced Xin (and myself) that the crash (double free) he was 
observing, and which he wanted to fix with the "tipc: fix a race in 
tipc_sk_mcast_rcv" patch was due to this bug.
Now, realizing that this is causing a memory leak and not a double free 
I suspect there might still be an issue.
Does anybody have a theory?

///jon

>>
>> Fixes: 6bf24dc0cc0c ("net:tipc: Fix a double free in tipc_sk_mcast_rcv")
>> Acked-by: Jon Maloy <jm...@re...>
>> Acked-by: Tung Nguyen <tun...@de...>
>> Signed-off-by: Hoang Le <hoa...@de...>
>>
>> [...]
> Here is the summary with links:
>    - [net] Revert "net:tipc: Fix a double free in tipc_sk_mcast_rcv"
>      https://git.kernel.org/netdev/net/c/75016891357a
>
> You are awesome, thank you!
> --
> Deet-doot-dot, I am a bot.
> https://korg.docs.kernel.org/patchwork/pwbot.html
>
>

[tipc-discussion] [PATCH net] tipc: simplify the finalize work queue

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

This patch is to use "struct work_struct" for the finalize work queue
instead of "struct tipc_net_work", as it can get the "net" and "addr"
from tipc_net's other members and there is no need to add extra net
and addr in tipc_net by defining "struct tipc_net_work".

Note that it's safe to get net from tn->bcl as bcl is always released
after the finalize work queue is done.

Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/core.c     |  4 ++--
 net/tipc/core.h     |  8 +-------
 net/tipc/discover.c |  4 ++--
 net/tipc/link.c     |  5 +++++
 net/tipc/link.h     |  1 +
 net/tipc/net.c      | 15 +++------------
 6 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/net/tipc/core.c b/net/tipc/core.c
index 72f3ac7..3f4542e 100644
--- a/net/tipc/core.c
+++ b/net/tipc/core.c
@@ -60,7 +60,7 @@ static int __net_init tipc_init_net(struct net *net)
 	tn->trial_addr = 0;
 	tn->addr_trial_end = 0;
 	tn->capabilities = TIPC_NODE_CAPABILITIES;
-	INIT_WORK(&tn->final_work.work, tipc_net_finalize_work);
+	INIT_WORK(&tn->work, tipc_net_finalize_work);
 	memset(tn->node_id, 0, sizeof(tn->node_id));
 	memset(tn->node_id_string, 0, sizeof(tn->node_id_string));
 	tn->mon_threshold = TIPC_DEF_MON_THRESHOLD;
@@ -110,7 +110,7 @@ static void __net_exit tipc_exit_net(struct net *net)
 
 	tipc_detach_loopback(net);
 	/* Make sure the tipc_net_finalize_work() finished */
-	cancel_work_sync(&tn->final_work.work);
+	cancel_work_sync(&tn->work);
 	tipc_net_stop(net);
 
 	tipc_bcast_stop(net);
diff --git a/net/tipc/core.h b/net/tipc/core.h
index 5741ae4..0a3f7a7 100644
--- a/net/tipc/core.h
+++ b/net/tipc/core.h
@@ -91,12 +91,6 @@ extern unsigned int tipc_net_id __read_mostly;
 extern int sysctl_tipc_rmem[3] __read_mostly;
 extern int sysctl_tipc_named_timeout __read_mostly;
 
-struct tipc_net_work {
-	struct work_struct work;
-	struct net *net;
-	u32 addr;
-};
-
 struct tipc_net {
 	u8  node_id[NODE_ID_LEN];
 	u32 node_addr;
@@ -148,7 +142,7 @@ struct tipc_net {
 	struct tipc_crypto *crypto_tx;
 #endif
 	/* Work item for net finalize */
-	struct tipc_net_work final_work;
+	struct work_struct work;
 	/* The numbers of work queues in schedule */
 	atomic_t wq_count;
 };
diff --git a/net/tipc/discover.c b/net/tipc/discover.c
index 5380f60..da69e1a 100644
--- a/net/tipc/discover.c
+++ b/net/tipc/discover.c
@@ -168,7 +168,7 @@ static bool tipc_disc_addr_trial_msg(struct tipc_discoverer *d,
 
 	/* Apply trial address if we just left trial period */
 	if (!trial && !self) {
-		tipc_sched_net_finalize(net, tn->trial_addr);
+		schedule_work(&tn->work);
 		msg_set_prevnode(buf_msg(d->skb), tn->trial_addr);
 		msg_set_type(buf_msg(d->skb), DSC_REQ_MSG);
 	}
@@ -308,7 +308,7 @@ static void tipc_disc_timeout(struct timer_list *t)
 	if (!time_before(jiffies, tn->addr_trial_end) && !tipc_own_addr(net)) {
 		mod_timer(&d->timer, jiffies + TIPC_DISC_INIT);
 		spin_unlock_bh(&d->lock);
-		tipc_sched_net_finalize(net, tn->trial_addr);
+		schedule_work(&tn->work);
 		return;
 	}
 
diff --git a/net/tipc/link.c b/net/tipc/link.c
index 1151092..c44b4bf 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -372,6 +372,11 @@ char tipc_link_plane(struct tipc_link *l)
 	return l->net_plane;
 }
 
+struct net *tipc_link_net(struct tipc_link *l)
+{
+	return l->net;
+}
+
 void tipc_link_update_caps(struct tipc_link *l, u16 capabilities)
 {
 	l->peer_caps = capabilities;
diff --git a/net/tipc/link.h b/net/tipc/link.h
index fc07232..a16f401 100644
--- a/net/tipc/link.h
+++ b/net/tipc/link.h
@@ -156,4 +156,5 @@ int tipc_link_bc_sync_rcv(struct tipc_link *l,   struct tipc_msg *hdr,
 int tipc_link_bc_nack_rcv(struct tipc_link *l, struct sk_buff *skb,
 			  struct sk_buff_head *xmitq);
 bool tipc_link_too_silent(struct tipc_link *l);
+struct net *tipc_link_net(struct tipc_link *l);
 #endif
diff --git a/net/tipc/net.c b/net/tipc/net.c
index a130195..0e95572 100644
--- a/net/tipc/net.c
+++ b/net/tipc/net.c
@@ -41,6 +41,7 @@
 #include "socket.h"
 #include "node.h"
 #include "bcast.h"
+#include "link.h"
 #include "netlink.h"
 #include "monitor.h"
 
@@ -142,19 +143,9 @@ static void tipc_net_finalize(struct net *net, u32 addr)
 
 void tipc_net_finalize_work(struct work_struct *work)
 {
-	struct tipc_net_work *fwork;
+	struct tipc_net *tn = container_of(work, struct tipc_net, work);
 
-	fwork = container_of(work, struct tipc_net_work, work);
-	tipc_net_finalize(fwork->net, fwork->addr);
-}
-
-void tipc_sched_net_finalize(struct net *net, u32 addr)
-{
-	struct tipc_net *tn = tipc_net(net);
-
-	tn->final_work.net = net;
-	tn->final_work.addr = addr;
-	schedule_work(&tn->final_work.work);
+	tipc_net_finalize(tipc_link_net(tn->bcl), tn->trial_addr);
 }
 
 void tipc_net_stop(struct net *net)
-- 
2.1.0

Re: [tipc-discussion] [PATCH net] tipc: fix a race in tipc_sk_mcast_rcv

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

On 5/14/21 11:42 AM, Xin Long wrote:
> On Thu, May 13, 2021 at 5:15 PM Jon Maloy <jm...@re...> wrote:
>>
>>
>> On 4/28/21 3:30 PM, Xin Long wrote:
>>> After commit cb1b728096f5 ("tipc: eliminate race condition at multicast
>>> reception"), when processing the multicast reception, the packets are
>>> firstly moved from be->inputq1 to be->arrvq in tipc_node_broadcast(),
>>> then process be->arrvq in tipc_sk_mcast_rcv().
>>>
>>> In tipc_sk_mcast_rcv(), it gets the 1st skb by skb_peek(), then process
>>> this skb without any lock. It means meanwhile another thread could also
>>> call tipc_sk_mcast_rcv() and process be->arrvq and pick up the same skb,
>>> then free it. A double free issue will be caused as Li Shuang reported:
>>>
>>>     [] kernel BUG at mm/slub.c:305!
>>>     []  kfree+0x3a7/0x3d0
>>>     []  kfree_skb+0x32/0xa0
>>>     []  skb_release_data+0xb4/0x170
>>>     []  kfree_skb+0x32/0xa0
>>>     []  skb_release_data+0xb4/0x170
>>>     []  kfree_skb+0x32/0xa0
>>>     []  tipc_sk_mcast_rcv+0x1fa/0x380 [tipc]
>>>     []  tipc_rcv+0x411/0x1120 [tipc]
>>>     []  tipc_udp_recv+0xc6/0x1e0 [tipc]
>>>     []  udp_queue_rcv_one_skb+0x1a9/0x500
>>>     []  udp_unicast_rcv_skb.isra.66+0x75/0x90
>>>     []  __udp4_lib_rcv+0x537/0xc40
>>>     []  ip_protocol_deliver_rcu+0xdf/0x1d0
>>>     []  ip_local_deliver_finish+0x4a/0x50
>>>     []  ip_local_deliver+0x6b/0xe0
>>>     []  ip_rcv+0x27b/0x36a
>>>     []  __netif_receive_skb_core+0xb47/0xc40
>>>     []  process_backlog+0xae/0x160
>>>
>>> Commit 6bf24dc0cc0c ("net:tipc: Fix a double free in tipc_sk_mcast_rcv")
>>> tried to fix this double free by not releasing the skbs in be->arrvq,
>>> which would definitely cause the skbs' leak.
>>>
>>> The problem is we shouldn't process the skbs in be->arrvq without any
>>> lock to protect the code from peeking to dequeuing them. The fix here
>>> is to use a temp skb list instead of be->arrvq to make it "per thread
>>> safe". While at it, remove the no-longer-used be->arrvq.
>>>
>>> Fixes: cb1b728096f5 ("tipc: eliminate race condition at multicast reception")
>>> Fixes: 6bf24dc0cc0c ("net:tipc: Fix a double free in tipc_sk_mcast_rcv")
>>> Reported-by: Li Shuang <sh...@re...>
>>> Signed-off-by: Xin Long <luc...@gm...>
>>> ---
>>>    net/tipc/node.c   |  9 ++++-----
>>>    net/tipc/socket.c | 16 +++-------------
>>>    2 files changed, 7 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/net/tipc/node.c b/net/tipc/node.c
>>> index e0ee832..0c636fb 100644
>>> --- a/net/tipc/node.c
>>> +++ b/net/tipc/node.c
>>> @@ -72,7 +72,6 @@ struct tipc_link_entry {
>>>    struct tipc_bclink_entry {
>>>        struct tipc_link *link;
>>>        struct sk_buff_head inputq1;
>>> -     struct sk_buff_head arrvq;
>>>        struct sk_buff_head inputq2;
>>>        struct sk_buff_head namedq;
>>>        u16 named_rcv_nxt;
>>> @@ -552,7 +551,6 @@ struct tipc_node *tipc_node_create(struct net *net, u32 addr, u8 *peer_id,
>>>        INIT_LIST_HEAD(&n->conn_sks);
>>>        skb_queue_head_init(&n->bc_entry.namedq);
>>>        skb_queue_head_init(&n->bc_entry.inputq1);
>>> -     __skb_queue_head_init(&n->bc_entry.arrvq);
>>>        skb_queue_head_init(&n->bc_entry.inputq2);
>>>        for (i = 0; i < MAX_BEARERS; i++)
>>>                spin_lock_init(&n->links[i].lock);
>>> @@ -1803,14 +1801,15 @@ void tipc_node_broadcast(struct net *net, struct sk_buff *skb, int rc_dests)
>>>    static void tipc_node_mcast_rcv(struct tipc_node *n)
>>>    {
>>>        struct tipc_bclink_entry *be = &n->bc_entry;
>>> +     struct sk_buff_head tmpq;
>>>
>>> -     /* 'arrvq' is under inputq2's lock protection */
>>> +     __skb_queue_head_init(&tmpq);
>>>        spin_lock_bh(&be->inputq2.lock);
>>>        spin_lock_bh(&be->inputq1.lock);
>>> -     skb_queue_splice_tail_init(&be->inputq1, &be->arrvq);
>>> +     skb_queue_splice_tail_init(&be->inputq1, &tmpq);
>>>        spin_unlock_bh(&be->inputq1.lock);
>>>        spin_unlock_bh(&be->inputq2.lock);
>>> -     tipc_sk_mcast_rcv(n->net, &be->arrvq, &be->inputq2);
>>> +     tipc_sk_mcast_rcv(n->net, &tmpq, &be->inputq2);
>>>    }
>>>
>>>    static void tipc_node_bc_sync_rcv(struct tipc_node *n, struct tipc_msg *hdr,
>>> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
>>> index 022999e..2870798 100644
>>> --- a/net/tipc/socket.c
>>> +++ b/net/tipc/socket.c
>>> @@ -1210,8 +1210,7 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
>>>        __skb_queue_head_init(&tmpq);
>>>        INIT_LIST_HEAD(&dports);
>>>
>>> -     skb = tipc_skb_peek(arrvq, &inputq->lock);
>>> -     for (; skb; skb = tipc_skb_peek(arrvq, &inputq->lock)) {
>>> +     while ((skb = __skb_dequeue(arrvq)) != NULL) {
>>>                hdr = buf_msg(skb);
>>>                user = msg_user(hdr);
>>>                mtyp = msg_type(hdr);
>>> @@ -1220,13 +1219,7 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
>>>                type = msg_nametype(hdr);
>>>
>>>                if (mtyp == TIPC_GRP_UCAST_MSG || user == GROUP_PROTOCOL) {
>>> -                     spin_lock_bh(&inputq->lock);
>>> -                     if (skb_peek(arrvq) == skb) {
>>> -                             __skb_dequeue(arrvq);
>>> -                             __skb_queue_tail(inputq, skb);
>>> -                     }
>>> -                     kfree_skb(skb);
>>> -                     spin_unlock_bh(&inputq->lock);
>>> +                     skb_queue_tail(inputq, skb);
>>>                        continue;
>>>                }
>>>
>>> @@ -1263,10 +1256,7 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
>>>                }
>>>                /* Append to inputq if not already done by other thread */
>>>                spin_lock_bh(&inputq->lock);
>>> -             if (skb_peek(arrvq) == skb) {
>>> -                     skb_queue_splice_tail_init(&tmpq, inputq);
>>> -                     __skb_dequeue(arrvq);
>>> -             }
>>> +             skb_queue_splice_tail_init(&tmpq, inputq);
>>>                spin_unlock_bh(&inputq->lock);
>>>                __skb_queue_purge(&tmpq);
>>>                kfree_skb(skb);
>> Nack.
>>
>> This would invalidate the sequence guarantee of messages between two
>> specific sockets.
>> The whole point of having a lock protected arrival queue is to preserve
>> the order when messages are moved from inputq1 to inputq2.
>> Let's take a discussion on our mailing list.
>>
> Hi, Jon, thanks for checking this.
>
> I'm making this tipc-discussion only.
> The problem you're saying exists even without this patch.
> unless we lock it until this dequeued skb enter into the sk's receive queue,
> something like:
>
> lock()
> skb=dequeue(arrv)
> ...
> tipc_sk_rcv(skb)
> unlock()
Not needed in my view. The fact that we hold both locks when we move a 
message from inputq1 to arrvq guarantees that they are placed there in 
order.
Likewise, when we move a message from arrvq to inputq2 we also hold a 
lock protecting both queues, so they cannot be disordered there either.

The trick is that we do peek instead of dequeue at the beginning of the 
loop.

Imagine the following:

Thread 1               Thread 2
-----------                 -------------

msg1 -> inputq1
msg1->arrvq
                           msg2->inputq1
                           msg2->arrvq
                           msg1->inputq2 // Because it is first in arrvq!!!
msg2->inputq2

So, whichever thread comes first will add the first message in the 
arrival queue to inputq1, no matter which one itself arrived with.
Does this make sense?

Also, the scenario you describe in the commit log cannot happen, since 
the "tipc_skb_peek()" call isn't an ordinary peek.
It even increments the skb's reference counter, so that if another 
thread picks it up and "frees" it there is still no double free.

What is really happening is that you have encountered the bug introduced by
the erroneous commit 6bf24dc0cc0c ("tipc: Fix a double free in 
tipc_sk_mcast_rcv¨).

Hoang has just posted a reversal of that commit, so you don't need to do 
anything.

BR
///jon

>
> that's also what other protocols are doing, and the bad side is less
> parallel processing.
>

[tipc-discussion] [PATCH net] tipc: wait and exit until all work queues are done

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

On some host, a crash could be triggered simply by repeating these
commands several times:

  # modprobe tipc
  # tipc bearer enable media udp name UDP1 localip 127.0.0.1
  # rmmod tipc

  [] BUG: unable to handle kernel paging request at ffffffffc096bb00
  [] Workqueue: events 0xffffffffc096bb00
  [] Call Trace:
  []  ? process_one_work+0x1a7/0x360
  []  ? worker_thread+0x30/0x390
  []  ? create_worker+0x1a0/0x1a0
  []  ? kthread+0x116/0x130
  []  ? kthread_flush_work_fn+0x10/0x10
  []  ? ret_from_fork+0x35/0x40

When removing the TIPC module, the UDP tunnel sock will be delayed to
release in a work queue as sock_release() can't be done in rtnl_lock().
If the work queue is schedule to run after the TIPC module is removed,
kernel will crash as the work queue function cleanup_beareri() code no
longer exists when trying to invoke it.

To fix it, this patch introduce a member wq_count in tipc_net to track
the numbers of work queues in schedule, and  wait and exit until all
work queues are done in tipc_exit_net().

Reported-by: Shuang Li <sh...@re...>
Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/core.c      | 2 ++
 net/tipc/core.h      | 2 ++
 net/tipc/udp_media.c | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/net/tipc/core.c b/net/tipc/core.c
index 5cc1f03..72f3ac7 100644
--- a/net/tipc/core.c
+++ b/net/tipc/core.c
@@ -119,6 +119,8 @@ static void __net_exit tipc_exit_net(struct net *net)
 #ifdef CONFIG_TIPC_CRYPTO
 	tipc_crypto_stop(&tipc_net(net)->crypto_tx);
 #endif
+	while (atomic_read(&tn->wq_count))
+		cond_resched();
 }
 
 static void __net_exit tipc_pernet_pre_exit(struct net *net)
diff --git a/net/tipc/core.h b/net/tipc/core.h
index 03de7b2..5741ae4 100644
--- a/net/tipc/core.h
+++ b/net/tipc/core.h
@@ -149,6 +149,8 @@ struct tipc_net {
 #endif
 	/* Work item for net finalize */
 	struct tipc_net_work final_work;
+	/* The numbers of work queues in schedule */
+	atomic_t wq_count;
 };
 
 static inline struct tipc_net *tipc_net(struct net *net)
diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
index e556d2c..c2bb818 100644
--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c
@@ -814,6 +814,7 @@ static void cleanup_bearer(struct work_struct *work)
 		kfree_rcu(rcast, rcu);
 	}
 
+	atomic_dec(&tipc_net(sock_net(ub->ubsock->sk))->wq_count);
 	dst_cache_destroy(&ub->rcast.dst_cache);
 	udp_tunnel_sock_release(ub->ubsock);
 	synchronize_net();
@@ -834,6 +835,7 @@ static void tipc_udp_disable(struct tipc_bearer *b)
 	RCU_INIT_POINTER(ub->bearer, NULL);
 
 	/* sock_release need to be done outside of rtnl lock */
+	atomic_inc(&tipc_net(sock_net(ub->ubsock->sk))->wq_count);
 	INIT_WORK(&ub->work, cleanup_bearer);
 	schedule_work(&ub->work);
 }
-- 
2.1.0

Re: [tipc-discussion] [PATCH net] tipc: fix a race in tipc_sk_mcast_rcv

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

On Thu, May 13, 2021 at 5:15 PM Jon Maloy <jm...@re...> wrote:
>
>
>
> On 4/28/21 3:30 PM, Xin Long wrote:
> > After commit cb1b728096f5 ("tipc: eliminate race condition at multicast
> > reception"), when processing the multicast reception, the packets are
> > firstly moved from be->inputq1 to be->arrvq in tipc_node_broadcast(),
> > then process be->arrvq in tipc_sk_mcast_rcv().
> >
> > In tipc_sk_mcast_rcv(), it gets the 1st skb by skb_peek(), then process
> > this skb without any lock. It means meanwhile another thread could also
> > call tipc_sk_mcast_rcv() and process be->arrvq and pick up the same skb,
> > then free it. A double free issue will be caused as Li Shuang reported:
> >
> >    [] kernel BUG at mm/slub.c:305!
> >    []  kfree+0x3a7/0x3d0
> >    []  kfree_skb+0x32/0xa0
> >    []  skb_release_data+0xb4/0x170
> >    []  kfree_skb+0x32/0xa0
> >    []  skb_release_data+0xb4/0x170
> >    []  kfree_skb+0x32/0xa0
> >    []  tipc_sk_mcast_rcv+0x1fa/0x380 [tipc]
> >    []  tipc_rcv+0x411/0x1120 [tipc]
> >    []  tipc_udp_recv+0xc6/0x1e0 [tipc]
> >    []  udp_queue_rcv_one_skb+0x1a9/0x500
> >    []  udp_unicast_rcv_skb.isra.66+0x75/0x90
> >    []  __udp4_lib_rcv+0x537/0xc40
> >    []  ip_protocol_deliver_rcu+0xdf/0x1d0
> >    []  ip_local_deliver_finish+0x4a/0x50
> >    []  ip_local_deliver+0x6b/0xe0
> >    []  ip_rcv+0x27b/0x36a
> >    []  __netif_receive_skb_core+0xb47/0xc40
> >    []  process_backlog+0xae/0x160
> >
> > Commit 6bf24dc0cc0c ("net:tipc: Fix a double free in tipc_sk_mcast_rcv")
> > tried to fix this double free by not releasing the skbs in be->arrvq,
> > which would definitely cause the skbs' leak.
> >
> > The problem is we shouldn't process the skbs in be->arrvq without any
> > lock to protect the code from peeking to dequeuing them. The fix here
> > is to use a temp skb list instead of be->arrvq to make it "per thread
> > safe". While at it, remove the no-longer-used be->arrvq.
> >
> > Fixes: cb1b728096f5 ("tipc: eliminate race condition at multicast reception")
> > Fixes: 6bf24dc0cc0c ("net:tipc: Fix a double free in tipc_sk_mcast_rcv")
> > Reported-by: Li Shuang <sh...@re...>
> > Signed-off-by: Xin Long <luc...@gm...>
> > ---
> >   net/tipc/node.c   |  9 ++++-----
> >   net/tipc/socket.c | 16 +++-------------
> >   2 files changed, 7 insertions(+), 18 deletions(-)
> >
> > diff --git a/net/tipc/node.c b/net/tipc/node.c
> > index e0ee832..0c636fb 100644
> > --- a/net/tipc/node.c
> > +++ b/net/tipc/node.c
> > @@ -72,7 +72,6 @@ struct tipc_link_entry {
> >   struct tipc_bclink_entry {
> >       struct tipc_link *link;
> >       struct sk_buff_head inputq1;
> > -     struct sk_buff_head arrvq;
> >       struct sk_buff_head inputq2;
> >       struct sk_buff_head namedq;
> >       u16 named_rcv_nxt;
> > @@ -552,7 +551,6 @@ struct tipc_node *tipc_node_create(struct net *net, u32 addr, u8 *peer_id,
> >       INIT_LIST_HEAD(&n->conn_sks);
> >       skb_queue_head_init(&n->bc_entry.namedq);
> >       skb_queue_head_init(&n->bc_entry.inputq1);
> > -     __skb_queue_head_init(&n->bc_entry.arrvq);
> >       skb_queue_head_init(&n->bc_entry.inputq2);
> >       for (i = 0; i < MAX_BEARERS; i++)
> >               spin_lock_init(&n->links[i].lock);
> > @@ -1803,14 +1801,15 @@ void tipc_node_broadcast(struct net *net, struct sk_buff *skb, int rc_dests)
> >   static void tipc_node_mcast_rcv(struct tipc_node *n)
> >   {
> >       struct tipc_bclink_entry *be = &n->bc_entry;
> > +     struct sk_buff_head tmpq;
> >
> > -     /* 'arrvq' is under inputq2's lock protection */
> > +     __skb_queue_head_init(&tmpq);
> >       spin_lock_bh(&be->inputq2.lock);
> >       spin_lock_bh(&be->inputq1.lock);
> > -     skb_queue_splice_tail_init(&be->inputq1, &be->arrvq);
> > +     skb_queue_splice_tail_init(&be->inputq1, &tmpq);
> >       spin_unlock_bh(&be->inputq1.lock);
> >       spin_unlock_bh(&be->inputq2.lock);
> > -     tipc_sk_mcast_rcv(n->net, &be->arrvq, &be->inputq2);
> > +     tipc_sk_mcast_rcv(n->net, &tmpq, &be->inputq2);
> >   }
> >
> >   static void tipc_node_bc_sync_rcv(struct tipc_node *n, struct tipc_msg *hdr,
> > diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> > index 022999e..2870798 100644
> > --- a/net/tipc/socket.c
> > +++ b/net/tipc/socket.c
> > @@ -1210,8 +1210,7 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
> >       __skb_queue_head_init(&tmpq);
> >       INIT_LIST_HEAD(&dports);
> >
> > -     skb = tipc_skb_peek(arrvq, &inputq->lock);
> > -     for (; skb; skb = tipc_skb_peek(arrvq, &inputq->lock)) {
> > +     while ((skb = __skb_dequeue(arrvq)) != NULL) {
> >               hdr = buf_msg(skb);
> >               user = msg_user(hdr);
> >               mtyp = msg_type(hdr);
> > @@ -1220,13 +1219,7 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
> >               type = msg_nametype(hdr);
> >
> >               if (mtyp == TIPC_GRP_UCAST_MSG || user == GROUP_PROTOCOL) {
> > -                     spin_lock_bh(&inputq->lock);
> > -                     if (skb_peek(arrvq) == skb) {
> > -                             __skb_dequeue(arrvq);
> > -                             __skb_queue_tail(inputq, skb);
> > -                     }
> > -                     kfree_skb(skb);
> > -                     spin_unlock_bh(&inputq->lock);
> > +                     skb_queue_tail(inputq, skb);
> >                       continue;
> >               }
> >
> > @@ -1263,10 +1256,7 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
> >               }
> >               /* Append to inputq if not already done by other thread */
> >               spin_lock_bh(&inputq->lock);
> > -             if (skb_peek(arrvq) == skb) {
> > -                     skb_queue_splice_tail_init(&tmpq, inputq);
> > -                     __skb_dequeue(arrvq);
> > -             }
> > +             skb_queue_splice_tail_init(&tmpq, inputq);
> >               spin_unlock_bh(&inputq->lock);
> >               __skb_queue_purge(&tmpq);
> >               kfree_skb(skb);
> Nack.
>
> This would invalidate the sequence guarantee of messages between two
> specific sockets.
> The whole point of having a lock protected arrival queue is to preserve
> the order when messages are moved from inputq1 to inputq2.
> Let's take a discussion on our mailing list.
>
Hi, Jon, thanks for checking this.

I'm making this tipc-discussion only.
The problem you're saying exists even without this patch.
unless we lock it until this dequeued skb enter into the sk's receive queue,
something like:

lock()
skb=dequeue(arrv)
...
tipc_sk_rcv(skb)
unlock()

that's also what other protocols are doing, and the bad side is less
parallel processing.

[tipc-discussion] [net] Revert "net:tipc: Fix a double free in tipc_sk_mcast_rcv"

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

This reverts commit 6bf24dc0cc0cc43b29ba344b66d78590e687e046.
Above fix is not correct and caused memory leak issue.

Fixes: 6bf24dc0cc0c ("net:tipc: Fix a double free in tipc_sk_mcast_rcv")
Acked-by: Jon Maloy <jm...@re...>
Acked-by: Tung Nguyen <tun...@de...>
Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/socket.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 58935cd0d068..53af72824c9c 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1262,7 +1262,10 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
 		spin_lock_bh(&inputq->lock);
 		if (skb_peek(arrvq) == skb) {
 			skb_queue_splice_tail_init(&tmpq, inputq);
-			__skb_dequeue(arrvq);
+			/* Decrease the skb's refcnt as increasing in the
+			 * function tipc_skb_peek
+			 */
+			kfree_skb(__skb_dequeue(arrvq));
 		}
 		spin_unlock_bh(&inputq->lock);
 		__skb_queue_purge(&tmpq);
-- 
2.25.1

Re: [tipc-discussion] [PATCH net] tipc: fix a race in tipc_sk_mcast_rcv

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

On 4/28/21 3:30 PM, Xin Long wrote:
> After commit cb1b728096f5 ("tipc: eliminate race condition at multicast
> reception"), when processing the multicast reception, the packets are
> firstly moved from be->inputq1 to be->arrvq in tipc_node_broadcast(),
> then process be->arrvq in tipc_sk_mcast_rcv().
>
> In tipc_sk_mcast_rcv(), it gets the 1st skb by skb_peek(), then process
> this skb without any lock. It means meanwhile another thread could also
> call tipc_sk_mcast_rcv() and process be->arrvq and pick up the same skb,
> then free it. A double free issue will be caused as Li Shuang reported:
>
>    [] kernel BUG at mm/slub.c:305!
>    []  kfree+0x3a7/0x3d0
>    []  kfree_skb+0x32/0xa0
>    []  skb_release_data+0xb4/0x170
>    []  kfree_skb+0x32/0xa0
>    []  skb_release_data+0xb4/0x170
>    []  kfree_skb+0x32/0xa0
>    []  tipc_sk_mcast_rcv+0x1fa/0x380 [tipc]
>    []  tipc_rcv+0x411/0x1120 [tipc]
>    []  tipc_udp_recv+0xc6/0x1e0 [tipc]
>    []  udp_queue_rcv_one_skb+0x1a9/0x500
>    []  udp_unicast_rcv_skb.isra.66+0x75/0x90
>    []  __udp4_lib_rcv+0x537/0xc40
>    []  ip_protocol_deliver_rcu+0xdf/0x1d0
>    []  ip_local_deliver_finish+0x4a/0x50
>    []  ip_local_deliver+0x6b/0xe0
>    []  ip_rcv+0x27b/0x36a
>    []  __netif_receive_skb_core+0xb47/0xc40
>    []  process_backlog+0xae/0x160
>
> Commit 6bf24dc0cc0c ("net:tipc: Fix a double free in tipc_sk_mcast_rcv")
> tried to fix this double free by not releasing the skbs in be->arrvq,
> which would definitely cause the skbs' leak.
>
> The problem is we shouldn't process the skbs in be->arrvq without any
> lock to protect the code from peeking to dequeuing them. The fix here
> is to use a temp skb list instead of be->arrvq to make it "per thread
> safe". While at it, remove the no-longer-used be->arrvq.
>
> Fixes: cb1b728096f5 ("tipc: eliminate race condition at multicast reception")
> Fixes: 6bf24dc0cc0c ("net:tipc: Fix a double free in tipc_sk_mcast_rcv")
> Reported-by: Li Shuang <sh...@re...>
> Signed-off-by: Xin Long <luc...@gm...>
> ---
>   net/tipc/node.c   |  9 ++++-----
>   net/tipc/socket.c | 16 +++-------------
>   2 files changed, 7 insertions(+), 18 deletions(-)
>
> diff --git a/net/tipc/node.c b/net/tipc/node.c
> index e0ee832..0c636fb 100644
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -72,7 +72,6 @@ struct tipc_link_entry {
>   struct tipc_bclink_entry {
>   	struct tipc_link *link;
>   	struct sk_buff_head inputq1;
> -	struct sk_buff_head arrvq;
>   	struct sk_buff_head inputq2;
>   	struct sk_buff_head namedq;
>   	u16 named_rcv_nxt;
> @@ -552,7 +551,6 @@ struct tipc_node *tipc_node_create(struct net *net, u32 addr, u8 *peer_id,
>   	INIT_LIST_HEAD(&n->conn_sks);
>   	skb_queue_head_init(&n->bc_entry.namedq);
>   	skb_queue_head_init(&n->bc_entry.inputq1);
> -	__skb_queue_head_init(&n->bc_entry.arrvq);
>   	skb_queue_head_init(&n->bc_entry.inputq2);
>   	for (i = 0; i < MAX_BEARERS; i++)
>   		spin_lock_init(&n->links[i].lock);
> @@ -1803,14 +1801,15 @@ void tipc_node_broadcast(struct net *net, struct sk_buff *skb, int rc_dests)
>   static void tipc_node_mcast_rcv(struct tipc_node *n)
>   {
>   	struct tipc_bclink_entry *be = &n->bc_entry;
> +	struct sk_buff_head tmpq;
>   
> -	/* 'arrvq' is under inputq2's lock protection */
> +	__skb_queue_head_init(&tmpq);
>   	spin_lock_bh(&be->inputq2.lock);
>   	spin_lock_bh(&be->inputq1.lock);
> -	skb_queue_splice_tail_init(&be->inputq1, &be->arrvq);
> +	skb_queue_splice_tail_init(&be->inputq1, &tmpq);
>   	spin_unlock_bh(&be->inputq1.lock);
>   	spin_unlock_bh(&be->inputq2.lock);
> -	tipc_sk_mcast_rcv(n->net, &be->arrvq, &be->inputq2);
> +	tipc_sk_mcast_rcv(n->net, &tmpq, &be->inputq2);
>   }
>   
>   static void tipc_node_bc_sync_rcv(struct tipc_node *n, struct tipc_msg *hdr,
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index 022999e..2870798 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -1210,8 +1210,7 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
>   	__skb_queue_head_init(&tmpq);
>   	INIT_LIST_HEAD(&dports);
>   
> -	skb = tipc_skb_peek(arrvq, &inputq->lock);
> -	for (; skb; skb = tipc_skb_peek(arrvq, &inputq->lock)) {
> +	while ((skb = __skb_dequeue(arrvq)) != NULL) {
>   		hdr = buf_msg(skb);
>   		user = msg_user(hdr);
>   		mtyp = msg_type(hdr);
> @@ -1220,13 +1219,7 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
>   		type = msg_nametype(hdr);
>   
>   		if (mtyp == TIPC_GRP_UCAST_MSG || user == GROUP_PROTOCOL) {
> -			spin_lock_bh(&inputq->lock);
> -			if (skb_peek(arrvq) == skb) {
> -				__skb_dequeue(arrvq);
> -				__skb_queue_tail(inputq, skb);
> -			}
> -			kfree_skb(skb);
> -			spin_unlock_bh(&inputq->lock);
> +			skb_queue_tail(inputq, skb);
>   			continue;
>   		}
>   
> @@ -1263,10 +1256,7 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
>   		}
>   		/* Append to inputq if not already done by other thread */
>   		spin_lock_bh(&inputq->lock);
> -		if (skb_peek(arrvq) == skb) {
> -			skb_queue_splice_tail_init(&tmpq, inputq);
> -			__skb_dequeue(arrvq);
> -		}
> +		skb_queue_splice_tail_init(&tmpq, inputq);
>   		spin_unlock_bh(&inputq->lock);
>   		__skb_queue_purge(&tmpq);
>   		kfree_skb(skb);
Nack.

This would invalidate the sequence guarantee of messages between two 
specific sockets.
The whole point of having a lock protected arrival queue is to preserve 
the order when messages are moved from inputq1 to inputq2.
Let's take a discussion on our mailing list.

///jon

[tipc-discussion] [net] tipc: make node link identity publish thread safe

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

The using of the node address and node link identity are not thread safe,
meaning that two publications may be published the same values, as result
one of them will get failure because of already existing in the name table.
To avoid this we have to use the node address and node link identity values
from inside the node item's write lock protection.

Fixes: 50a3499ab853 ("tipc: simplify signature of tipc_namtbl_publish()")
Acked-by: Jon Maloy <jm...@re...>
Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/node.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/tipc/node.c b/net/tipc/node.c
index 8217905348f4..81af92954c6c 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -423,18 +423,18 @@ static void tipc_node_write_unlock(struct tipc_node *n)
 	write_unlock_bh(&n->lock);
 
 	if (flags & TIPC_NOTIFY_NODE_DOWN)
-		tipc_publ_notify(net, publ_list, n->addr, n->capabilities);
+		tipc_publ_notify(net, publ_list, sk.node, n->capabilities);
 
 	if (flags & TIPC_NOTIFY_NODE_UP)
-		tipc_named_node_up(net, n->addr, n->capabilities);
+		tipc_named_node_up(net, sk.node, n->capabilities);
 
 	if (flags & TIPC_NOTIFY_LINK_UP) {
-		tipc_mon_peer_up(net, n->addr, bearer_id);
-		tipc_nametbl_publish(net, &ua, &sk, n->link_id);
+		tipc_mon_peer_up(net, sk.node, bearer_id);
+		tipc_nametbl_publish(net, &ua, &sk, sk.ref);
 	}
 	if (flags & TIPC_NOTIFY_LINK_DOWN) {
-		tipc_mon_peer_down(net, n->addr, bearer_id);
-		tipc_nametbl_withdraw(net, &ua, &sk, n->link_id);
+		tipc_mon_peer_down(net, sk.node, bearer_id);
+		tipc_nametbl_withdraw(net, &ua, &sk, sk.ref);
 	}
 }
 
-- 
2.25.1

Re: [tipc-discussion] [iproute2] tipc: call a sub-routine in separate socket

[David A. <ds...@gm...>]

On 5/5/21 9:27 PM, Hoang Le wrote:
> When receiving a result from first query to netlink, we may exec
> a another query inside the callback. If calling this sub-routine
> in the same socket, it will be discarded the result from previous
> exection.
> To avoid this we perform a nested query in separate socket.
> 
> Fixes: 202102830663 ("tipc: use the libmnl functions in lib/mnl_utils.c")
> Signed-off-by: Hoang Le <hoa...@de...>
> Acked-by: Jon Maloy <jm...@re...>
> ---
>  tipc/bearer.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
>  tipc/link.c   | 15 +++++++++++++--
>  tipc/socket.c | 17 +++++++++++++++--
>  3 files changed, 73 insertions(+), 9 deletions(-)
> 

applied, thanks

[tipc-discussion] [PATCH net] tipc: skb_linearize the head skb when reassembling msgs

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

It's not a good idea to append the frag skb to a skb's frag_list if
the frag_list already has skbs from elsewhere, such as this skb was
created by pskb_copy() where the frag_list was cloned (all the skbs
in it were skb_get'ed) and shared by multiple skbs.

However, the new appended frag skb should have been only seen by the
current skb. Otherwise, it will cause use after free crashes as this
appended frag skb are seen by multiple skbs but it only got skb_get
called once.

The same thing happens with a skb updated by pskb_may_pull() with a
skb_cloned skb. Li Shuang has reported quite a few crashes caused
by this when doing testing over macvlan devices:

  [] kernel BUG at net/core/skbuff.c:1970!
  [] Call Trace:
  []  skb_clone+0x4d/0xb0
  []  macvlan_broadcast+0xd8/0x160 [macvlan]
  []  macvlan_process_broadcast+0x148/0x150 [macvlan]
  []  process_one_work+0x1a7/0x360
  []  worker_thread+0x30/0x390

  [] kernel BUG at mm/usercopy.c:102!
  [] Call Trace:
  []  __check_heap_object+0xd3/0x100
  []  __check_object_size+0xff/0x16b
  []  simple_copy_to_iter+0x1c/0x30
  []  __skb_datagram_iter+0x7d/0x310
  []  __skb_datagram_iter+0x2a5/0x310
  []  skb_copy_datagram_iter+0x3b/0x90
  []  tipc_recvmsg+0x14a/0x3a0 [tipc]
  []  ____sys_recvmsg+0x91/0x150
  []  ___sys_recvmsg+0x7b/0xc0

  [] kernel BUG at mm/slub.c:305!
  [] Call Trace:
  []  <IRQ>
  []  kmem_cache_free+0x3ff/0x400
  []  __netif_receive_skb_core+0x12c/0xc40
  []  ? kmem_cache_alloc+0x12e/0x270
  []  netif_receive_skb_internal+0x3d/0xb0
  []  ? get_rx_page_info+0x8e/0xa0 [be2net]
  []  be_poll+0x6ef/0xd00 [be2net]
  []  ? irq_exit+0x4f/0x100
  []  net_rx_action+0x149/0x3b0

  ...

This patch is to fix it by linearizing the head skb if it has frag_list
set in tipc_buf_append(). Note that we choose to do this before calling
skb_unshare(), as __skb_linearize() will avoid skb_copy(). Also, we can
not just drop the frag_list either as the early time.

Fixes: 45c8b7b175ce ("tipc: allow non-linear first fragment buffer")
Reported-by: Li Shuang <sh...@re...>
Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/msg.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 3f0a253..ce6ab54 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -149,18 +149,13 @@ int tipc_buf_append(struct sk_buff **headbuf, struct sk_buff **buf)
 		if (unlikely(head))
 			goto err;
 		*buf = NULL;
+		if (skb_has_frag_list(frag) && __skb_linearize(frag))
+			goto err;
 		frag = skb_unshare(frag, GFP_ATOMIC);
 		if (unlikely(!frag))
 			goto err;
 		head = *headbuf = frag;
 		TIPC_SKB_CB(head)->tail = NULL;
-		if (skb_is_nonlinear(head)) {
-			skb_walk_frags(head, tail) {
-				TIPC_SKB_CB(head)->tail = tail;
-			}
-		} else {
-			skb_frag_list_init(head);
-		}
 		return 0;
 	}
 
-- 
2.1.0

[tipc-discussion] [iproute2] tipc: call a sub-routine in separate socket

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

When receiving a result from first query to netlink, we may exec
a another query inside the callback. If calling this sub-routine
in the same socket, it will be discarded the result from previous
exection.
To avoid this we perform a nested query in separate socket.

Fixes: 202102830663 ("tipc: use the libmnl functions in lib/mnl_utils.c")
Signed-off-by: Hoang Le <hoa...@de...>
Acked-by: Jon Maloy <jm...@re...>
---
 tipc/bearer.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
 tipc/link.c   | 15 +++++++++++++--
 tipc/socket.c | 17 +++++++++++++++--
 3 files changed, 73 insertions(+), 9 deletions(-)

diff --git a/tipc/bearer.c b/tipc/bearer.c
index 2afc48b9b108..968293bc9160 100644
--- a/tipc/bearer.c
+++ b/tipc/bearer.c
@@ -20,7 +20,9 @@
 #include <linux/tipc.h>
 #include <linux/genetlink.h>
 #include <linux/if.h>
+#include <libmnl/libmnl.h>
 
+#include "mnl_utils.h"
 #include "utils.h"
 #include "cmdl.h"
 #include "msg.h"
@@ -98,16 +100,28 @@ static int get_netid_cb(const struct nlmsghdr *nlh, void *data)
 
 static int generate_multicast(short af, char *buf, int bufsize)
 {
+	struct mnlu_gen_socket bearer_nlg;
 	struct nlmsghdr *nlh;
 	int netid;
+	int err = 0;
 
-	nlh = msg_init(TIPC_NL_NET_GET);
+	err = mnlu_gen_socket_open(&bearer_nlg, TIPC_GENL_V2_NAME,
+				   TIPC_GENL_V2_VERSION);
+	if (err)
+		return -1;
+
+	nlh = mnlu_gen_socket_cmd_prepare(&bearer_nlg, TIPC_NL_NET_GET,
+					  NLM_F_REQUEST | NLM_F_DUMP);
 	if (!nlh) {
 		fprintf(stderr, "error, message initialization failed\n");
+		mnlu_gen_socket_close(&bearer_nlg);
 		return -1;
 	}
-	if (msg_dumpit(nlh, get_netid_cb, &netid)) {
+
+	err = mnlu_gen_socket_sndrcv(&bearer_nlg, nlh, get_netid_cb, &netid);
+	if (err) {
 		fprintf(stderr, "error, failed to fetch TIPC network id from kernel\n");
+		mnlu_gen_socket_close(&bearer_nlg);
 		return -EINVAL;
 	}
 	if (af == AF_INET)
@@ -115,6 +129,7 @@ static int generate_multicast(short af, char *buf, int bufsize)
 	else
 		snprintf(buf, bufsize, "ff02::%u", netid);
 
+	mnlu_gen_socket_close(&bearer_nlg);
 	return 0;
 }
 
@@ -794,10 +809,35 @@ static int bearer_get_udp_cb(const struct nlmsghdr *nlh, void *data)
 	if ((cb_data->attr == TIPC_NLA_UDP_REMOTE) &&
 	    (cb_data->prop == UDP_PROP_IP) &&
 	    opts[TIPC_NLA_UDP_MULTI_REMOTEIP]) {
-		struct genlmsghdr *genl = mnl_nlmsg_get_payload(cb_data->nlh);
+		struct mnlu_gen_socket bearer_nlg;
+		struct nlattr *attr;
+		struct nlmsghdr *h;
+		const char *bname;
+		int err = 0;
+
+		err = mnlu_gen_socket_open(&bearer_nlg, TIPC_GENL_V2_NAME,
+					   TIPC_GENL_V2_VERSION);
+		if (err)
+			return -1;
+
+		h = mnlu_gen_socket_cmd_prepare(&bearer_nlg,
+						TIPC_NL_UDP_GET_REMOTEIP,
+						NLM_F_REQUEST | NLM_F_DUMP);
+		if (!h) {
+			fprintf(stderr, "error, message initialization failed\n");
+			mnlu_gen_socket_close(&bearer_nlg);
+			return -1;
+		}
 
-		genl->cmd = TIPC_NL_UDP_GET_REMOTEIP;
-		return msg_dumpit(cb_data->nlh, bearer_dump_udp_cb, NULL);
+		attr = mnl_attr_nest_start(h, TIPC_NLA_BEARER);
+		bname = mnl_attr_get_str(attrs[TIPC_NLA_BEARER_NAME]);
+		mnl_attr_put_strz(h, TIPC_NLA_BEARER_NAME, bname);
+		mnl_attr_nest_end(h, attr);
+
+		err = mnlu_gen_socket_sndrcv(&bearer_nlg, h,
+					     bearer_dump_udp_cb, NULL);
+		mnlu_gen_socket_close(&bearer_nlg);
+		return err;
 	}
 
 	addr = mnl_attr_get_payload(opts[cb_data->attr]);
diff --git a/tipc/link.c b/tipc/link.c
index 2123f109c694..9994ada2a367 100644
--- a/tipc/link.c
+++ b/tipc/link.c
@@ -17,7 +17,9 @@
 #include <linux/tipc_netlink.h>
 #include <linux/tipc.h>
 #include <linux/genetlink.h>
+#include <libmnl/libmnl.h>
 
+#include "mnl_utils.h"
 #include "cmdl.h"
 #include "msg.h"
 #include "link.h"
@@ -993,13 +995,20 @@ exit:
 
 static int link_mon_peer_list(uint32_t mon_ref)
 {
+	struct mnlu_gen_socket link_nlg;
 	struct nlmsghdr *nlh;
 	struct nlattr *nest;
 	int err = 0;
 
-	nlh = msg_init(TIPC_NL_MON_PEER_GET);
+	err = mnlu_gen_socket_open(&link_nlg, TIPC_GENL_V2_NAME,
+				   TIPC_GENL_V2_VERSION);
+	if (err)
+		return -1;
+	nlh = mnlu_gen_socket_cmd_prepare(&link_nlg, TIPC_NL_MON_PEER_GET,
+					  NLM_F_REQUEST | NLM_F_DUMP);
 	if (!nlh) {
 		fprintf(stderr, "error, message initialisation failed\n");
+		mnlu_gen_socket_close(&link_nlg);
 		return -1;
 	}
 
@@ -1007,7 +1016,9 @@ static int link_mon_peer_list(uint32_t mon_ref)
 	mnl_attr_put_u32(nlh, TIPC_NLA_MON_REF, mon_ref);
 	mnl_attr_nest_end(nlh, nest);
 
-	err = msg_dumpit(nlh, link_mon_peer_list_cb, NULL);
+	err = mnlu_gen_socket_sndrcv(&link_nlg, nlh, link_mon_peer_list_cb,
+				     NULL);
+	mnlu_gen_socket_close(&link_nlg);
 	return err;
 }
 
diff --git a/tipc/socket.c b/tipc/socket.c
index deae12af4409..597ffd91af52 100644
--- a/tipc/socket.c
+++ b/tipc/socket.c
@@ -15,7 +15,9 @@
 #include <linux/tipc.h>
 #include <linux/tipc_netlink.h>
 #include <linux/genetlink.h>
+#include <libmnl/libmnl.h>
 
+#include "mnl_utils.h"
 #include "cmdl.h"
 #include "msg.h"
 #include "socket.h"
@@ -44,12 +46,21 @@ static int publ_list_cb(const struct nlmsghdr *nlh, void *data)
 
 static int publ_list(uint32_t sock)
 {
+	struct mnlu_gen_socket sock_nlg;
 	struct nlmsghdr *nlh;
 	struct nlattr *nest;
+	int err;
 
-	nlh = msg_init(TIPC_NL_PUBL_GET);
+	err = mnlu_gen_socket_open(&sock_nlg, TIPC_GENL_V2_NAME,
+				   TIPC_GENL_V2_VERSION);
+	if (err)
+		return -1;
+
+	nlh = mnlu_gen_socket_cmd_prepare(&sock_nlg, TIPC_NL_PUBL_GET,
+					  NLM_F_REQUEST | NLM_F_DUMP);
 	if (!nlh) {
 		fprintf(stderr, "error, message initialisation failed\n");
+		mnlu_gen_socket_close(&sock_nlg);
 		return -1;
 	}
 
@@ -57,7 +68,9 @@ static int publ_list(uint32_t sock)
 	mnl_attr_put_u32(nlh, TIPC_NLA_SOCK_REF, sock);
 	mnl_attr_nest_end(nlh, nest);
 
-	return msg_dumpit(nlh, publ_list_cb, NULL);
+	err = mnlu_gen_socket_sndrcv(&sock_nlg, nlh, publ_list_cb, NULL);
+	mnlu_gen_socket_close(&sock_nlg);
+	return err;
 }
 
 static int sock_list_cb(const struct nlmsghdr *nlh, void *data)
-- 
2.25.1

Re: [tipc-discussion] [iproute2-next v2] tipc: call a sub-routine in separate socket

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

On 5/4/21 12:56 AM, Hoang Le wrote:
> When receiving a result from first query to netlink, we may exec
> a another query inside the callback. If calling this sub-routine
> in the same socket, it will be discarded the result from previous
> exection.
> To avoid this we perform a nested query in separate socket.
>
> Fixes: 202102830663 ("tipc: use the libmnl functions in lib/mnl_utils.c")
> Signed-off-by: Hoang Le <hoa...@de...>
> ---
>   tipc/bearer.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
>   tipc/link.c   | 15 +++++++++++++--
>   tipc/socket.c | 17 +++++++++++++++--
>   3 files changed, 73 insertions(+), 9 deletions(-)
>
> diff --git a/tipc/bearer.c b/tipc/bearer.c
> index 2afc48b9b108..968293bc9160 100644
> --- a/tipc/bearer.c
> +++ b/tipc/bearer.c
> @@ -20,7 +20,9 @@
>   #include <linux/tipc.h>
>   #include <linux/genetlink.h>
>   #include <linux/if.h>
> +#include <libmnl/libmnl.h>
>   
> +#include "mnl_utils.h"
>   #include "utils.h"
>   #include "cmdl.h"
>   #include "msg.h"
> @@ -98,16 +100,28 @@ static int get_netid_cb(const struct nlmsghdr *nlh, void *data)
>   
>   static int generate_multicast(short af, char *buf, int bufsize)
>   {
> +	struct mnlu_gen_socket bearer_nlg;
>   	struct nlmsghdr *nlh;
>   	int netid;
> +	int err = 0;
>   
> -	nlh = msg_init(TIPC_NL_NET_GET);
> +	err = mnlu_gen_socket_open(&bearer_nlg, TIPC_GENL_V2_NAME,
> +				   TIPC_GENL_V2_VERSION);
> +	if (err)
> +		return -1;
> +
> +	nlh = mnlu_gen_socket_cmd_prepare(&bearer_nlg, TIPC_NL_NET_GET,
> +					  NLM_F_REQUEST | NLM_F_DUMP);
>   	if (!nlh) {
>   		fprintf(stderr, "error, message initialization failed\n");
> +		mnlu_gen_socket_close(&bearer_nlg);
>   		return -1;
>   	}
> -	if (msg_dumpit(nlh, get_netid_cb, &netid)) {
> +
> +	err = mnlu_gen_socket_sndrcv(&bearer_nlg, nlh, get_netid_cb, &netid);
> +	if (err) {
>   		fprintf(stderr, "error, failed to fetch TIPC network id from kernel\n");
> +		mnlu_gen_socket_close(&bearer_nlg);
>   		return -EINVAL;
>   	}
>   	if (af == AF_INET)
> @@ -115,6 +129,7 @@ static int generate_multicast(short af, char *buf, int bufsize)
>   	else
>   		snprintf(buf, bufsize, "ff02::%u", netid);
>   
> +	mnlu_gen_socket_close(&bearer_nlg);
>   	return 0;
>   }
>   
> @@ -794,10 +809,35 @@ static int bearer_get_udp_cb(const struct nlmsghdr *nlh, void *data)
>   	if ((cb_data->attr == TIPC_NLA_UDP_REMOTE) &&
>   	    (cb_data->prop == UDP_PROP_IP) &&
>   	    opts[TIPC_NLA_UDP_MULTI_REMOTEIP]) {
> -		struct genlmsghdr *genl = mnl_nlmsg_get_payload(cb_data->nlh);
> +		struct mnlu_gen_socket bearer_nlg;
> +		struct nlattr *attr;
> +		struct nlmsghdr *h;
> +		const char *bname;
> +		int err = 0;
> +
> +		err = mnlu_gen_socket_open(&bearer_nlg, TIPC_GENL_V2_NAME,
> +					   TIPC_GENL_V2_VERSION);
> +		if (err)
> +			return -1;
> +
> +		h = mnlu_gen_socket_cmd_prepare(&bearer_nlg,
> +						TIPC_NL_UDP_GET_REMOTEIP,
> +						NLM_F_REQUEST | NLM_F_DUMP);
> +		if (!h) {
> +			fprintf(stderr, "error, message initialization failed\n");
> +			mnlu_gen_socket_close(&bearer_nlg);
> +			return -1;
> +		}
>   
> -		genl->cmd = TIPC_NL_UDP_GET_REMOTEIP;
> -		return msg_dumpit(cb_data->nlh, bearer_dump_udp_cb, NULL);
> +		attr = mnl_attr_nest_start(h, TIPC_NLA_BEARER);
> +		bname = mnl_attr_get_str(attrs[TIPC_NLA_BEARER_NAME]);
> +		mnl_attr_put_strz(h, TIPC_NLA_BEARER_NAME, bname);
> +		mnl_attr_nest_end(h, attr);
> +
> +		err = mnlu_gen_socket_sndrcv(&bearer_nlg, h,
> +					     bearer_dump_udp_cb, NULL);
> +		mnlu_gen_socket_close(&bearer_nlg);
> +		return err;
>   	}
>   
>   	addr = mnl_attr_get_payload(opts[cb_data->attr]);
> diff --git a/tipc/link.c b/tipc/link.c
> index 2123f109c694..9994ada2a367 100644
> --- a/tipc/link.c
> +++ b/tipc/link.c
> @@ -17,7 +17,9 @@
>   #include <linux/tipc_netlink.h>
>   #include <linux/tipc.h>
>   #include <linux/genetlink.h>
> +#include <libmnl/libmnl.h>
>   
> +#include "mnl_utils.h"
>   #include "cmdl.h"
>   #include "msg.h"
>   #include "link.h"
> @@ -993,13 +995,20 @@ exit:
>   
>   static int link_mon_peer_list(uint32_t mon_ref)
>   {
> +	struct mnlu_gen_socket link_nlg;
>   	struct nlmsghdr *nlh;
>   	struct nlattr *nest;
>   	int err = 0;
>   
> -	nlh = msg_init(TIPC_NL_MON_PEER_GET);
> +	err = mnlu_gen_socket_open(&link_nlg, TIPC_GENL_V2_NAME,
> +				   TIPC_GENL_V2_VERSION);
> +	if (err)
> +		return -1;
> +	nlh = mnlu_gen_socket_cmd_prepare(&link_nlg, TIPC_NL_MON_PEER_GET,
> +					  NLM_F_REQUEST | NLM_F_DUMP);
>   	if (!nlh) {
>   		fprintf(stderr, "error, message initialisation failed\n");
> +		mnlu_gen_socket_close(&link_nlg);
>   		return -1;
>   	}
>   
> @@ -1007,7 +1016,9 @@ static int link_mon_peer_list(uint32_t mon_ref)
>   	mnl_attr_put_u32(nlh, TIPC_NLA_MON_REF, mon_ref);
>   	mnl_attr_nest_end(nlh, nest);
>   
> -	err = msg_dumpit(nlh, link_mon_peer_list_cb, NULL);
> +	err = mnlu_gen_socket_sndrcv(&link_nlg, nlh, link_mon_peer_list_cb,
> +				     NULL);
> +	mnlu_gen_socket_close(&link_nlg);
>   	return err;
>   }
>   
> diff --git a/tipc/socket.c b/tipc/socket.c
> index deae12af4409..597ffd91af52 100644
> --- a/tipc/socket.c
> +++ b/tipc/socket.c
> @@ -15,7 +15,9 @@
>   #include <linux/tipc.h>
>   #include <linux/tipc_netlink.h>
>   #include <linux/genetlink.h>
> +#include <libmnl/libmnl.h>
>   
> +#include "mnl_utils.h"
>   #include "cmdl.h"
>   #include "msg.h"
>   #include "socket.h"
> @@ -44,12 +46,21 @@ static int publ_list_cb(const struct nlmsghdr *nlh, void *data)
>   
>   static int publ_list(uint32_t sock)
>   {
> +	struct mnlu_gen_socket sock_nlg;
>   	struct nlmsghdr *nlh;
>   	struct nlattr *nest;
> +	int err;
>   
> -	nlh = msg_init(TIPC_NL_PUBL_GET);
> +	err = mnlu_gen_socket_open(&sock_nlg, TIPC_GENL_V2_NAME,
> +				   TIPC_GENL_V2_VERSION);
> +	if (err)
> +		return -1;
> +
> +	nlh = mnlu_gen_socket_cmd_prepare(&sock_nlg, TIPC_NL_PUBL_GET,
> +					  NLM_F_REQUEST | NLM_F_DUMP);
>   	if (!nlh) {
>   		fprintf(stderr, "error, message initialisation failed\n");
> +		mnlu_gen_socket_close(&sock_nlg);
>   		return -1;
>   	}
>   
> @@ -57,7 +68,9 @@ static int publ_list(uint32_t sock)
>   	mnl_attr_put_u32(nlh, TIPC_NLA_SOCK_REF, sock);
>   	mnl_attr_nest_end(nlh, nest);
>   
> -	return msg_dumpit(nlh, publ_list_cb, NULL);
> +	err = mnlu_gen_socket_sndrcv(&sock_nlg, nlh, publ_list_cb, NULL);
> +	mnlu_gen_socket_close(&sock_nlg);
> +	return err;
>   }
>   
>   static int sock_list_cb(const struct nlmsghdr *nlh, void *data)
Looks good.
Acked-by: Jon Maloy <jm...@re...>

[tipc-discussion] [iproute2-next v2] tipc: call a sub-routine in separate socket

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

When receiving a result from first query to netlink, we may exec
a another query inside the callback. If calling this sub-routine
in the same socket, it will be discarded the result from previous
exection.
To avoid this we perform a nested query in separate socket.

Fixes: 202102830663 ("tipc: use the libmnl functions in lib/mnl_utils.c")
Signed-off-by: Hoang Le <hoa...@de...>
---
 tipc/bearer.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
 tipc/link.c   | 15 +++++++++++++--
 tipc/socket.c | 17 +++++++++++++++--
 3 files changed, 73 insertions(+), 9 deletions(-)

diff --git a/tipc/bearer.c b/tipc/bearer.c
index 2afc48b9b108..968293bc9160 100644
--- a/tipc/bearer.c
+++ b/tipc/bearer.c
@@ -20,7 +20,9 @@
 #include <linux/tipc.h>
 #include <linux/genetlink.h>
 #include <linux/if.h>
+#include <libmnl/libmnl.h>
 
+#include "mnl_utils.h"
 #include "utils.h"
 #include "cmdl.h"
 #include "msg.h"
@@ -98,16 +100,28 @@ static int get_netid_cb(const struct nlmsghdr *nlh, void *data)
 
 static int generate_multicast(short af, char *buf, int bufsize)
 {
+	struct mnlu_gen_socket bearer_nlg;
 	struct nlmsghdr *nlh;
 	int netid;
+	int err = 0;
 
-	nlh = msg_init(TIPC_NL_NET_GET);
+	err = mnlu_gen_socket_open(&bearer_nlg, TIPC_GENL_V2_NAME,
+				   TIPC_GENL_V2_VERSION);
+	if (err)
+		return -1;
+
+	nlh = mnlu_gen_socket_cmd_prepare(&bearer_nlg, TIPC_NL_NET_GET,
+					  NLM_F_REQUEST | NLM_F_DUMP);
 	if (!nlh) {
 		fprintf(stderr, "error, message initialization failed\n");
+		mnlu_gen_socket_close(&bearer_nlg);
 		return -1;
 	}
-	if (msg_dumpit(nlh, get_netid_cb, &netid)) {
+
+	err = mnlu_gen_socket_sndrcv(&bearer_nlg, nlh, get_netid_cb, &netid);
+	if (err) {
 		fprintf(stderr, "error, failed to fetch TIPC network id from kernel\n");
+		mnlu_gen_socket_close(&bearer_nlg);
 		return -EINVAL;
 	}
 	if (af == AF_INET)
@@ -115,6 +129,7 @@ static int generate_multicast(short af, char *buf, int bufsize)
 	else
 		snprintf(buf, bufsize, "ff02::%u", netid);
 
+	mnlu_gen_socket_close(&bearer_nlg);
 	return 0;
 }
 
@@ -794,10 +809,35 @@ static int bearer_get_udp_cb(const struct nlmsghdr *nlh, void *data)
 	if ((cb_data->attr == TIPC_NLA_UDP_REMOTE) &&
 	    (cb_data->prop == UDP_PROP_IP) &&
 	    opts[TIPC_NLA_UDP_MULTI_REMOTEIP]) {
-		struct genlmsghdr *genl = mnl_nlmsg_get_payload(cb_data->nlh);
+		struct mnlu_gen_socket bearer_nlg;
+		struct nlattr *attr;
+		struct nlmsghdr *h;
+		const char *bname;
+		int err = 0;
+
+		err = mnlu_gen_socket_open(&bearer_nlg, TIPC_GENL_V2_NAME,
+					   TIPC_GENL_V2_VERSION);
+		if (err)
+			return -1;
+
+		h = mnlu_gen_socket_cmd_prepare(&bearer_nlg,
+						TIPC_NL_UDP_GET_REMOTEIP,
+						NLM_F_REQUEST | NLM_F_DUMP);
+		if (!h) {
+			fprintf(stderr, "error, message initialization failed\n");
+			mnlu_gen_socket_close(&bearer_nlg);
+			return -1;
+		}
 
-		genl->cmd = TIPC_NL_UDP_GET_REMOTEIP;
-		return msg_dumpit(cb_data->nlh, bearer_dump_udp_cb, NULL);
+		attr = mnl_attr_nest_start(h, TIPC_NLA_BEARER);
+		bname = mnl_attr_get_str(attrs[TIPC_NLA_BEARER_NAME]);
+		mnl_attr_put_strz(h, TIPC_NLA_BEARER_NAME, bname);
+		mnl_attr_nest_end(h, attr);
+
+		err = mnlu_gen_socket_sndrcv(&bearer_nlg, h,
+					     bearer_dump_udp_cb, NULL);
+		mnlu_gen_socket_close(&bearer_nlg);
+		return err;
 	}
 
 	addr = mnl_attr_get_payload(opts[cb_data->attr]);
diff --git a/tipc/link.c b/tipc/link.c
index 2123f109c694..9994ada2a367 100644
--- a/tipc/link.c
+++ b/tipc/link.c
@@ -17,7 +17,9 @@
 #include <linux/tipc_netlink.h>
 #include <linux/tipc.h>
 #include <linux/genetlink.h>
+#include <libmnl/libmnl.h>
 
+#include "mnl_utils.h"
 #include "cmdl.h"
 #include "msg.h"
 #include "link.h"
@@ -993,13 +995,20 @@ exit:
 
 static int link_mon_peer_list(uint32_t mon_ref)
 {
+	struct mnlu_gen_socket link_nlg;
 	struct nlmsghdr *nlh;
 	struct nlattr *nest;
 	int err = 0;
 
-	nlh = msg_init(TIPC_NL_MON_PEER_GET);
+	err = mnlu_gen_socket_open(&link_nlg, TIPC_GENL_V2_NAME,
+				   TIPC_GENL_V2_VERSION);
+	if (err)
+		return -1;
+	nlh = mnlu_gen_socket_cmd_prepare(&link_nlg, TIPC_NL_MON_PEER_GET,
+					  NLM_F_REQUEST | NLM_F_DUMP);
 	if (!nlh) {
 		fprintf(stderr, "error, message initialisation failed\n");
+		mnlu_gen_socket_close(&link_nlg);
 		return -1;
 	}
 
@@ -1007,7 +1016,9 @@ static int link_mon_peer_list(uint32_t mon_ref)
 	mnl_attr_put_u32(nlh, TIPC_NLA_MON_REF, mon_ref);
 	mnl_attr_nest_end(nlh, nest);
 
-	err = msg_dumpit(nlh, link_mon_peer_list_cb, NULL);
+	err = mnlu_gen_socket_sndrcv(&link_nlg, nlh, link_mon_peer_list_cb,
+				     NULL);
+	mnlu_gen_socket_close(&link_nlg);
 	return err;
 }
 
diff --git a/tipc/socket.c b/tipc/socket.c
index deae12af4409..597ffd91af52 100644
--- a/tipc/socket.c
+++ b/tipc/socket.c
@@ -15,7 +15,9 @@
 #include <linux/tipc.h>
 #include <linux/tipc_netlink.h>
 #include <linux/genetlink.h>
+#include <libmnl/libmnl.h>
 
+#include "mnl_utils.h"
 #include "cmdl.h"
 #include "msg.h"
 #include "socket.h"
@@ -44,12 +46,21 @@ static int publ_list_cb(const struct nlmsghdr *nlh, void *data)
 
 static int publ_list(uint32_t sock)
 {
+	struct mnlu_gen_socket sock_nlg;
 	struct nlmsghdr *nlh;
 	struct nlattr *nest;
+	int err;
 
-	nlh = msg_init(TIPC_NL_PUBL_GET);
+	err = mnlu_gen_socket_open(&sock_nlg, TIPC_GENL_V2_NAME,
+				   TIPC_GENL_V2_VERSION);
+	if (err)
+		return -1;
+
+	nlh = mnlu_gen_socket_cmd_prepare(&sock_nlg, TIPC_NL_PUBL_GET,
+					  NLM_F_REQUEST | NLM_F_DUMP);
 	if (!nlh) {
 		fprintf(stderr, "error, message initialisation failed\n");
+		mnlu_gen_socket_close(&sock_nlg);
 		return -1;
 	}
 
@@ -57,7 +68,9 @@ static int publ_list(uint32_t sock)
 	mnl_attr_put_u32(nlh, TIPC_NLA_SOCK_REF, sock);
 	mnl_attr_nest_end(nlh, nest);
 
-	return msg_dumpit(nlh, publ_list_cb, NULL);
+	err = mnlu_gen_socket_sndrcv(&sock_nlg, nlh, publ_list_cb, NULL);
+	mnlu_gen_socket_close(&sock_nlg);
+	return err;
 }
 
 static int sock_list_cb(const struct nlmsghdr *nlh, void *data)
-- 
2.25.1

[tipc-discussion] [PATCHv2 net] tipc: fix a race in tipc_sk_mcast_rcv

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

After commit cb1b728096f5 ("tipc: eliminate race condition at multicast
reception"), when processing the multicast reception, the packets are
firstly moved from be->inputq1 to be->arrvq in tipc_node_broadcast(),
then it processes be->arrvq in tipc_sk_mcast_rcv().

In tipc_sk_mcast_rcv(), it gets the 1st skb by skb_peek(), then handles
this skb without any lock. It means meanwhile another thread could also
call tipc_sk_mcast_rcv() and process be->arrvq and pick up the same skb,
then free it. A double free issue will be caused as Li Shuang reported:

  [] kernel BUG at mm/slub.c:305!
  []  kfree+0x3a7/0x3d0
  []  kfree_skb+0x32/0xa0
  []  skb_release_data+0xb4/0x170
  []  kfree_skb+0x32/0xa0
  []  skb_release_data+0xb4/0x170
  []  kfree_skb+0x32/0xa0
  []  tipc_sk_mcast_rcv+0x1fa/0x380 [tipc]
  []  tipc_rcv+0x411/0x1120 [tipc]
  []  tipc_udp_recv+0xc6/0x1e0 [tipc]
  []  udp_queue_rcv_one_skb+0x1a9/0x500
  []  udp_unicast_rcv_skb.isra.66+0x75/0x90
  []  __udp4_lib_rcv+0x537/0xc40
  []  ip_protocol_deliver_rcu+0xdf/0x1d0
  []  ip_local_deliver_finish+0x4a/0x50
  []  ip_local_deliver+0x6b/0xe0
  []  ip_rcv+0x27b/0x36a
  []  __netif_receive_skb_core+0xb47/0xc40
  []  process_backlog+0xae/0x160

Commit 6bf24dc0cc0c ("net:tipc: Fix a double free in tipc_sk_mcast_rcv")
tried to fix this double free by not releasing the skbs in be->arrvq,
which would definitely cause the skbs' leak.

The problem is we shouldn't process the skbs in be->arrvq without any
lock to protect the code from peeking to dequeuing them. The fix here
is to use a temp skb list instead of be->arrvq to make it "per thread
safe". While at it, remove the no-longer-used be->arrvq.

v1->v2:
  - remove the no-longer-used tipc_skb_peek() and some comments from
    tipc_sk_mcast_rcv() as Tung noticed.

Fixes: cb1b728096f5 ("tipc: eliminate race condition at multicast reception")
Fixes: 6bf24dc0cc0c ("net:tipc: Fix a double free in tipc_sk_mcast_rcv")
Reported-by: Li Shuang <sh...@re...>
Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/msg.h    | 17 -----------------
 net/tipc/node.c   |  9 ++++-----
 net/tipc/socket.c | 17 +++--------------
 3 files changed, 7 insertions(+), 36 deletions(-)

diff --git a/net/tipc/msg.h b/net/tipc/msg.h
index 5d64596..7914358 100644
--- a/net/tipc/msg.h
+++ b/net/tipc/msg.h
@@ -1213,23 +1213,6 @@ static inline int buf_roundup_len(struct sk_buff *skb)
 	return (skb->len / 1024 + 1) * 1024;
 }
 
-/* tipc_skb_peek(): peek and reserve first buffer in list
- * @list: list to be peeked in
- * Returns pointer to first buffer in list, if any
- */
-static inline struct sk_buff *tipc_skb_peek(struct sk_buff_head *list,
-					    spinlock_t *lock)
-{
-	struct sk_buff *skb;
-
-	spin_lock_bh(lock);
-	skb = skb_peek(list);
-	if (skb)
-		skb_get(skb);
-	spin_unlock_bh(lock);
-	return skb;
-}
-
 /* tipc_skb_peek_port(): find a destination port, ignoring all destinations
  *                       up to and including 'filter'.
  * Note: ignoring previously tried destinations minimizes the risk of
diff --git a/net/tipc/node.c b/net/tipc/node.c
index e0ee832..0c636fb 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -72,7 +72,6 @@ struct tipc_link_entry {
 struct tipc_bclink_entry {
 	struct tipc_link *link;
 	struct sk_buff_head inputq1;
-	struct sk_buff_head arrvq;
 	struct sk_buff_head inputq2;
 	struct sk_buff_head namedq;
 	u16 named_rcv_nxt;
@@ -552,7 +551,6 @@ struct tipc_node *tipc_node_create(struct net *net, u32 addr, u8 *peer_id,
 	INIT_LIST_HEAD(&n->conn_sks);
 	skb_queue_head_init(&n->bc_entry.namedq);
 	skb_queue_head_init(&n->bc_entry.inputq1);
-	__skb_queue_head_init(&n->bc_entry.arrvq);
 	skb_queue_head_init(&n->bc_entry.inputq2);
 	for (i = 0; i < MAX_BEARERS; i++)
 		spin_lock_init(&n->links[i].lock);
@@ -1803,14 +1801,15 @@ void tipc_node_broadcast(struct net *net, struct sk_buff *skb, int rc_dests)
 static void tipc_node_mcast_rcv(struct tipc_node *n)
 {
 	struct tipc_bclink_entry *be = &n->bc_entry;
+	struct sk_buff_head tmpq;
 
-	/* 'arrvq' is under inputq2's lock protection */
+	__skb_queue_head_init(&tmpq);
 	spin_lock_bh(&be->inputq2.lock);
 	spin_lock_bh(&be->inputq1.lock);
-	skb_queue_splice_tail_init(&be->inputq1, &be->arrvq);
+	skb_queue_splice_tail_init(&be->inputq1, &tmpq);
 	spin_unlock_bh(&be->inputq1.lock);
 	spin_unlock_bh(&be->inputq2.lock);
-	tipc_sk_mcast_rcv(n->net, &be->arrvq, &be->inputq2);
+	tipc_sk_mcast_rcv(n->net, &tmpq, &be->inputq2);
 }
 
 static void tipc_node_bc_sync_rcv(struct tipc_node *n, struct tipc_msg *hdr,
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 022999e..cfd30fa 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1210,8 +1210,7 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
 	__skb_queue_head_init(&tmpq);
 	INIT_LIST_HEAD(&dports);
 
-	skb = tipc_skb_peek(arrvq, &inputq->lock);
-	for (; skb; skb = tipc_skb_peek(arrvq, &inputq->lock)) {
+	while ((skb = __skb_dequeue(arrvq)) != NULL) {
 		hdr = buf_msg(skb);
 		user = msg_user(hdr);
 		mtyp = msg_type(hdr);
@@ -1220,13 +1219,7 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
 		type = msg_nametype(hdr);
 
 		if (mtyp == TIPC_GRP_UCAST_MSG || user == GROUP_PROTOCOL) {
-			spin_lock_bh(&inputq->lock);
-			if (skb_peek(arrvq) == skb) {
-				__skb_dequeue(arrvq);
-				__skb_queue_tail(inputq, skb);
-			}
-			kfree_skb(skb);
-			spin_unlock_bh(&inputq->lock);
+			skb_queue_tail(inputq, skb);
 			continue;
 		}
 
@@ -1261,12 +1254,8 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
 			}
 			pr_warn("Failed to clone mcast rcv buffer\n");
 		}
-		/* Append to inputq if not already done by other thread */
 		spin_lock_bh(&inputq->lock);
-		if (skb_peek(arrvq) == skb) {
-			skb_queue_splice_tail_init(&tmpq, inputq);
-			__skb_dequeue(arrvq);
-		}
+		skb_queue_splice_tail_init(&tmpq, inputq);
 		spin_unlock_bh(&inputq->lock);
 		__skb_queue_purge(&tmpq);
 		kfree_skb(skb);
-- 
2.1.0

Re: [tipc-discussion] [PATCH net] tipc: fix a race in tipc_sk_mcast_rcv

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

Hi Xin,

See my comments inline.

Best regards,
Tung Nguyen

-----Original Message-----
From: Xin Long <luc...@gm...> 
Sent: Thursday, April 29, 2021 2:31 AM
To: network dev <ne...@vg...>; tip...@li...
Cc: ku...@ke...; ly...@ma...; da...@da...
Subject: [tipc-discussion] [PATCH net] tipc: fix a race in tipc_sk_mcast_rcv

After commit cb1b728096f5 ("tipc: eliminate race condition at multicast
reception"), when processing the multicast reception, the packets are
firstly moved from be->inputq1 to be->arrvq in tipc_node_broadcast(),
then process be->arrvq in tipc_sk_mcast_rcv().

In tipc_sk_mcast_rcv(), it gets the 1st skb by skb_peek(), then process
this skb without any lock. It means meanwhile another thread could also
call tipc_sk_mcast_rcv() and process be->arrvq and pick up the same skb,
then free it. A double free issue will be caused as Li Shuang reported:

  [] kernel BUG at mm/slub.c:305!
  []  kfree+0x3a7/0x3d0
  []  kfree_skb+0x32/0xa0
  []  skb_release_data+0xb4/0x170
  []  kfree_skb+0x32/0xa0
  []  skb_release_data+0xb4/0x170
  []  kfree_skb+0x32/0xa0
  []  tipc_sk_mcast_rcv+0x1fa/0x380 [tipc]
  []  tipc_rcv+0x411/0x1120 [tipc]
  []  tipc_udp_recv+0xc6/0x1e0 [tipc]
  []  udp_queue_rcv_one_skb+0x1a9/0x500
  []  udp_unicast_rcv_skb.isra.66+0x75/0x90
  []  __udp4_lib_rcv+0x537/0xc40
  []  ip_protocol_deliver_rcu+0xdf/0x1d0
  []  ip_local_deliver_finish+0x4a/0x50
  []  ip_local_deliver+0x6b/0xe0
  []  ip_rcv+0x27b/0x36a
  []  __netif_receive_skb_core+0xb47/0xc40
  []  process_backlog+0xae/0x160

Commit 6bf24dc0cc0c ("net:tipc: Fix a double free in tipc_sk_mcast_rcv")
tried to fix this double free by not releasing the skbs in be->arrvq,
which would definitely cause the skbs' leak.

The problem is we shouldn't process the skbs in be->arrvq without any
lock to protect the code from peeking to dequeuing them. The fix here
is to use a temp skb list instead of be->arrvq to make it "per thread
safe". While at it, remove the no-longer-used be->arrvq.

Fixes: cb1b728096f5 ("tipc: eliminate race condition at multicast reception")
Fixes: 6bf24dc0cc0c ("net:tipc: Fix a double free in tipc_sk_mcast_rcv")
Reported-by: Li Shuang <sh...@re...>
Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/node.c   |  9 ++++-----
 net/tipc/socket.c | 16 +++-------------
 2 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/net/tipc/node.c b/net/tipc/node.c
index e0ee832..0c636fb 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -72,7 +72,6 @@ struct tipc_link_entry {
 struct tipc_bclink_entry {
 	struct tipc_link *link;
 	struct sk_buff_head inputq1;
-	struct sk_buff_head arrvq;
 	struct sk_buff_head inputq2;
 	struct sk_buff_head namedq;
 	u16 named_rcv_nxt;
@@ -552,7 +551,6 @@ struct tipc_node *tipc_node_create(struct net *net, u32 addr, u8 *peer_id,
 	INIT_LIST_HEAD(&n->conn_sks);
 	skb_queue_head_init(&n->bc_entry.namedq);
 	skb_queue_head_init(&n->bc_entry.inputq1);
-	__skb_queue_head_init(&n->bc_entry.arrvq);
 	skb_queue_head_init(&n->bc_entry.inputq2);
 	for (i = 0; i < MAX_BEARERS; i++)
 		spin_lock_init(&n->links[i].lock);
@@ -1803,14 +1801,15 @@ void tipc_node_broadcast(struct net *net, struct sk_buff *skb, int rc_dests)
 static void tipc_node_mcast_rcv(struct tipc_node *n)
 {
 	struct tipc_bclink_entry *be = &n->bc_entry;
+	struct sk_buff_head tmpq;
 
-	/* 'arrvq' is under inputq2's lock protection */
+	__skb_queue_head_init(&tmpq);
 	spin_lock_bh(&be->inputq2.lock);
 	spin_lock_bh(&be->inputq1.lock);
-	skb_queue_splice_tail_init(&be->inputq1, &be->arrvq);
+	skb_queue_splice_tail_init(&be->inputq1, &tmpq);
 	spin_unlock_bh(&be->inputq1.lock);
 	spin_unlock_bh(&be->inputq2.lock);
-	tipc_sk_mcast_rcv(n->net, &be->arrvq, &be->inputq2);
+	tipc_sk_mcast_rcv(n->net, &tmpq, &be->inputq2);
 }
 
 static void tipc_node_bc_sync_rcv(struct tipc_node *n, struct tipc_msg *hdr,
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 022999e..2870798 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1210,8 +1210,7 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
 	__skb_queue_head_init(&tmpq);
 	INIT_LIST_HEAD(&dports);
 
-	skb = tipc_skb_peek(arrvq, &inputq->lock);
-	for (; skb; skb = tipc_skb_peek(arrvq, &inputq->lock)) {
Please remove function tipc_skb_peek() because it is no longer used.

+	while ((skb = __skb_dequeue(arrvq)) != NULL) {
 		hdr = buf_msg(skb);
 		user = msg_user(hdr);
 		mtyp = msg_type(hdr);
@@ -1220,13 +1219,7 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
 		type = msg_nametype(hdr);
 
 		if (mtyp == TIPC_GRP_UCAST_MSG || user == GROUP_PROTOCOL) {
-			spin_lock_bh(&inputq->lock);
-			if (skb_peek(arrvq) == skb) {
-				__skb_dequeue(arrvq);
-				__skb_queue_tail(inputq, skb);
-			}
-			kfree_skb(skb);
-			spin_unlock_bh(&inputq->lock);
+			skb_queue_tail(inputq, skb);
 			continue;
 		}
 
@@ -1263,10 +1256,7 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
 		}
 		/* Append to inputq if not already done by other thread */
Please remove above comment because the temporary queue is thread-safe
 		spin_lock_bh(&inputq->lock);
-		if (skb_peek(arrvq) == skb) {
-			skb_queue_splice_tail_init(&tmpq, inputq);
-			__skb_dequeue(arrvq);
-		}
+		skb_queue_splice_tail_init(&tmpq, inputq);
 		spin_unlock_bh(&inputq->lock);
 		__skb_queue_purge(&tmpq);
 		kfree_skb(skb);
-- 
2.1.0



_______________________________________________
tipc-discussion mailing list
tip...@li...
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [PATCH net] tipc: fix a race in tipc_sk_mcast_rcv

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

After commit cb1b728096f5 ("tipc: eliminate race condition at multicast
reception"), when processing the multicast reception, the packets are
firstly moved from be->inputq1 to be->arrvq in tipc_node_broadcast(),
then process be->arrvq in tipc_sk_mcast_rcv().

In tipc_sk_mcast_rcv(), it gets the 1st skb by skb_peek(), then process
this skb without any lock. It means meanwhile another thread could also
call tipc_sk_mcast_rcv() and process be->arrvq and pick up the same skb,
then free it. A double free issue will be caused as Li Shuang reported:

  [] kernel BUG at mm/slub.c:305!
  []  kfree+0x3a7/0x3d0
  []  kfree_skb+0x32/0xa0
  []  skb_release_data+0xb4/0x170
  []  kfree_skb+0x32/0xa0
  []  skb_release_data+0xb4/0x170
  []  kfree_skb+0x32/0xa0
  []  tipc_sk_mcast_rcv+0x1fa/0x380 [tipc]
  []  tipc_rcv+0x411/0x1120 [tipc]
  []  tipc_udp_recv+0xc6/0x1e0 [tipc]
  []  udp_queue_rcv_one_skb+0x1a9/0x500
  []  udp_unicast_rcv_skb.isra.66+0x75/0x90
  []  __udp4_lib_rcv+0x537/0xc40
  []  ip_protocol_deliver_rcu+0xdf/0x1d0
  []  ip_local_deliver_finish+0x4a/0x50
  []  ip_local_deliver+0x6b/0xe0
  []  ip_rcv+0x27b/0x36a
  []  __netif_receive_skb_core+0xb47/0xc40
  []  process_backlog+0xae/0x160

Commit 6bf24dc0cc0c ("net:tipc: Fix a double free in tipc_sk_mcast_rcv")
tried to fix this double free by not releasing the skbs in be->arrvq,
which would definitely cause the skbs' leak.

The problem is we shouldn't process the skbs in be->arrvq without any
lock to protect the code from peeking to dequeuing them. The fix here
is to use a temp skb list instead of be->arrvq to make it "per thread
safe". While at it, remove the no-longer-used be->arrvq.

Fixes: cb1b728096f5 ("tipc: eliminate race condition at multicast reception")
Fixes: 6bf24dc0cc0c ("net:tipc: Fix a double free in tipc_sk_mcast_rcv")
Reported-by: Li Shuang <sh...@re...>
Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/node.c   |  9 ++++-----
 net/tipc/socket.c | 16 +++-------------
 2 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/net/tipc/node.c b/net/tipc/node.c
index e0ee832..0c636fb 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -72,7 +72,6 @@ struct tipc_link_entry {
 struct tipc_bclink_entry {
 	struct tipc_link *link;
 	struct sk_buff_head inputq1;
-	struct sk_buff_head arrvq;
 	struct sk_buff_head inputq2;
 	struct sk_buff_head namedq;
 	u16 named_rcv_nxt;
@@ -552,7 +551,6 @@ struct tipc_node *tipc_node_create(struct net *net, u32 addr, u8 *peer_id,
 	INIT_LIST_HEAD(&n->conn_sks);
 	skb_queue_head_init(&n->bc_entry.namedq);
 	skb_queue_head_init(&n->bc_entry.inputq1);
-	__skb_queue_head_init(&n->bc_entry.arrvq);
 	skb_queue_head_init(&n->bc_entry.inputq2);
 	for (i = 0; i < MAX_BEARERS; i++)
 		spin_lock_init(&n->links[i].lock);
@@ -1803,14 +1801,15 @@ void tipc_node_broadcast(struct net *net, struct sk_buff *skb, int rc_dests)
 static void tipc_node_mcast_rcv(struct tipc_node *n)
 {
 	struct tipc_bclink_entry *be = &n->bc_entry;
+	struct sk_buff_head tmpq;
 
-	/* 'arrvq' is under inputq2's lock protection */
+	__skb_queue_head_init(&tmpq);
 	spin_lock_bh(&be->inputq2.lock);
 	spin_lock_bh(&be->inputq1.lock);
-	skb_queue_splice_tail_init(&be->inputq1, &be->arrvq);
+	skb_queue_splice_tail_init(&be->inputq1, &tmpq);
 	spin_unlock_bh(&be->inputq1.lock);
 	spin_unlock_bh(&be->inputq2.lock);
-	tipc_sk_mcast_rcv(n->net, &be->arrvq, &be->inputq2);
+	tipc_sk_mcast_rcv(n->net, &tmpq, &be->inputq2);
 }
 
 static void tipc_node_bc_sync_rcv(struct tipc_node *n, struct tipc_msg *hdr,
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 022999e..2870798 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1210,8 +1210,7 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
 	__skb_queue_head_init(&tmpq);
 	INIT_LIST_HEAD(&dports);
 
-	skb = tipc_skb_peek(arrvq, &inputq->lock);
-	for (; skb; skb = tipc_skb_peek(arrvq, &inputq->lock)) {
+	while ((skb = __skb_dequeue(arrvq)) != NULL) {
 		hdr = buf_msg(skb);
 		user = msg_user(hdr);
 		mtyp = msg_type(hdr);
@@ -1220,13 +1219,7 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
 		type = msg_nametype(hdr);
 
 		if (mtyp == TIPC_GRP_UCAST_MSG || user == GROUP_PROTOCOL) {
-			spin_lock_bh(&inputq->lock);
-			if (skb_peek(arrvq) == skb) {
-				__skb_dequeue(arrvq);
-				__skb_queue_tail(inputq, skb);
-			}
-			kfree_skb(skb);
-			spin_unlock_bh(&inputq->lock);
+			skb_queue_tail(inputq, skb);
 			continue;
 		}
 
@@ -1263,10 +1256,7 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
 		}
 		/* Append to inputq if not already done by other thread */
 		spin_lock_bh(&inputq->lock);
-		if (skb_peek(arrvq) == skb) {
-			skb_queue_splice_tail_init(&tmpq, inputq);
-			__skb_dequeue(arrvq);
-		}
+		skb_queue_splice_tail_init(&tmpq, inputq);
 		spin_unlock_bh(&inputq->lock);
 		__skb_queue_purge(&tmpq);
 		kfree_skb(skb);
-- 
2.1.0

Re: [tipc-discussion] [PATCH RESEND][next] tipc: Fix fall-through warnings for Clang

[Xue, Y. <Yin...@wi...>]

This patch looks good to me.

-----Original Message-----
From: Gustavo A. R. Silva <gus...@ke...> 
Sent: Friday, March 5, 2021 5:25 PM
To: Jon Maloy <jm...@re...>; Xue, Ying <Yin...@wi...>; David S. Miller <da...@da...>; Jakub Kicinski <ku...@ke...>
Cc: ne...@vg...; tip...@li...; lin...@vg...; Gustavo A. R. Silva <gus...@ke...>; lin...@vg...
Subject: [PATCH RESEND][next] tipc: Fix fall-through warnings for Clang

In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning by explicitly adding a break statement instead of letting the code fall through to the next case.

Link: https://github.com/KSPP/linux/issues/115
Signed-off-by: Gustavo A. R. Silva <gus...@ke...>
---
 net/tipc/link.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/tipc/link.c b/net/tipc/link.c index 115109259430..bcc426e16725 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -649,6 +649,7 @@ int tipc_link_fsm_evt(struct tipc_link *l, int evt)
 			break;
 		case LINK_FAILOVER_BEGIN_EVT:
 			l->state = LINK_FAILINGOVER;
+			break;
 		case LINK_FAILURE_EVT:
 		case LINK_RESET_EVT:
 		case LINK_ESTABLISH_EVT:
--
2.27.0

[tipc-discussion] [[re-send] net] tipc: make node link identity publish thread safe

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

The using of the node address and node link identity are not thread safe,
meaning that two publications may be published the same values, as result
one of them will get failure because of already existing in the name table.
To avoid this we have to use the node address and node link identity values
from inside the node item's write lock protection.

Fixes: 50a3499ab853 ("tipc: simplify signature of tipc_namtbl_publish()")
Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/node.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/tipc/node.c b/net/tipc/node.c
index 8217905348f4..81af92954c6c 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -423,18 +423,18 @@ static void tipc_node_write_unlock(struct tipc_node *n)
 	write_unlock_bh(&n->lock);
 
 	if (flags & TIPC_NOTIFY_NODE_DOWN)
-		tipc_publ_notify(net, publ_list, n->addr, n->capabilities);
+		tipc_publ_notify(net, publ_list, sk.node, n->capabilities);
 
 	if (flags & TIPC_NOTIFY_NODE_UP)
-		tipc_named_node_up(net, n->addr, n->capabilities);
+		tipc_named_node_up(net, sk.node, n->capabilities);
 
 	if (flags & TIPC_NOTIFY_LINK_UP) {
-		tipc_mon_peer_up(net, n->addr, bearer_id);
-		tipc_nametbl_publish(net, &ua, &sk, n->link_id);
+		tipc_mon_peer_up(net, sk.node, bearer_id);
+		tipc_nametbl_publish(net, &ua, &sk, sk.ref);
 	}
 	if (flags & TIPC_NOTIFY_LINK_DOWN) {
-		tipc_mon_peer_down(net, n->addr, bearer_id);
-		tipc_nametbl_withdraw(net, &ua, &sk, n->link_id);
+		tipc_mon_peer_down(net, sk.node, bearer_id);
+		tipc_nametbl_withdraw(net, &ua, &sk, sk.ref);
 	}
 }
 
-- 
2.25.1

[tipc-discussion] [net v2] Revert "net:tipc: Fix a double free in tipc_sk_mcast_rcv"

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

This reverts commit 6bf24dc0cc0cc43b29ba344b66d78590e687e046.
Above fix is not correct and caused memory leak issue.

Acked-by: Tung Nguyen <tun...@de...>
Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/socket.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 58935cd0d068..53af72824c9c 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1262,7 +1262,10 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
 		spin_lock_bh(&inputq->lock);
 		if (skb_peek(arrvq) == skb) {
 			skb_queue_splice_tail_init(&tmpq, inputq);
-			__skb_dequeue(arrvq);
+			/* Decrease the skb's refcnt as increasing in the
+			 * function tipc_skb_peek
+			 */
+			kfree_skb(__skb_dequeue(arrvq));
 		}
 		spin_unlock_bh(&inputq->lock);
 		__skb_queue_purge(&tmpq);
-- 
2.25.1

Re: [tipc-discussion] [net-next v2 0/3] tipc: some small cleanups

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

On Wed, Apr 7, 2021 at 4:59 PM <jm...@re...> wrote:
>
> From: Jon Maloy <jm...@re...>
>
> We make some minor code cleanups and improvements.
>
> ---
> v2: - Removed patch #1 from v1, which has now been applied upstream
>     - Fixed memory leak in patch #2 as identified by Hoang
>
> Jon Maloy (3):
>   tipc: eliminate redundant fields in struct tipc_sock
>   tipc: refactor function tipc_sk_anc_data_recv()
>   tipc: simplify handling of lookup scope during multicast message
>     reception
>
>  net/tipc/name_table.c |   6 +-
>  net/tipc/name_table.h |   4 +-
>  net/tipc/socket.c     | 149 +++++++++++++++++++-----------------------
>  3 files changed, 74 insertions(+), 85 deletions(-)
>
> --
> 2.29.2
>
>
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
LGTM!

Reviewed-by: Xin Long <luc...@gm...>

Re: [tipc-discussion] [net-next v2 0/3] tipc: some small cleanups

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

Series 
Tested-by: Hoang Le <hoa...@de...>

> -----Original Message-----
> From: jm...@re... <jm...@re...>
> Sent: Thursday, April 8, 2021 3:59 AM
> To: tip...@li...
> Cc: Tung Quang Nguyen <tun...@de...>; Hoang Huu Le <hoa...@de...>; Tuong Tong Lien
> <tuo...@de...>; jm...@re...; ma...@do...; lx...@re...; yin...@wi...;
> par...@gm...
> Subject: [net-next v2 0/3] tipc: some small cleanups
> 
> From: Jon Maloy <jm...@re...>
> 
> We make some minor code cleanups and improvements.
> 
> ---
> v2: - Removed patch #1 from v1, which has now been applied upstream
>     - Fixed memory leak in patch #2 as identified by Hoang
> 
> Jon Maloy (3):
>   tipc: eliminate redundant fields in struct tipc_sock
>   tipc: refactor function tipc_sk_anc_data_recv()
>   tipc: simplify handling of lookup scope during multicast message
>     reception
> 
>  net/tipc/name_table.c |   6 +-
>  net/tipc/name_table.h |   4 +-
>  net/tipc/socket.c     | 149 +++++++++++++++++++-----------------------
>  3 files changed, 74 insertions(+), 85 deletions(-)
> 
> --
> 2.29.2

Re: [tipc-discussion] [net] Revert "net:tipc: Fix a double free in tipc_sk_mcast_rcv"

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

Acked-by: Tung Nguyen <tun...@de...>

-----Original Message-----
From: Hoang Huu Le <hoa...@de...> 
Sent: Monday, April 12, 2021 4:02 PM
To: ly...@ma...; da...@da...; tip...@li...; jm...@re...; ma...@do...; yin...@wi...; Tung Quang Nguyen <tun...@de...>
Subject: [net] Revert "net:tipc: Fix a double free in tipc_sk_mcast_rcv"

This reverts commit 6bf24dc0cc0cc43b29ba344b66d78590e687e046.

Above fix is not correct and caused memory leak issue:
In the function tipc_skb_peek, skb's refcnt increasing. Then
we have to call kfree_skb twice to decrease skb's refcnt and
free a skb.

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

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 58935cd0d068..f21162aa0cf7 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1262,7 +1262,7 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
 		spin_lock_bh(&inputq->lock);
 		if (skb_peek(arrvq) == skb) {
 			skb_queue_splice_tail_init(&tmpq, inputq);
-			__skb_dequeue(arrvq);
+			kfree_skb(__skb_dequeue(arrvq));
 		}
 		spin_unlock_bh(&inputq->lock);
 		__skb_queue_purge(&tmpq);
-- 
2.25.1

[tipc-discussion] [net] Revert "net:tipc: Fix a double free in tipc_sk_mcast_rcv"

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

This reverts commit 6bf24dc0cc0cc43b29ba344b66d78590e687e046.

Above fix is not correct and caused memory leak issue:
In the function tipc_skb_peek, skb's refcnt increasing. Then
we have to call kfree_skb twice to decrease skb's refcnt and
free a skb.

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

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 58935cd0d068..f21162aa0cf7 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1262,7 +1262,7 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
 		spin_lock_bh(&inputq->lock);
 		if (skb_peek(arrvq) == skb) {
 			skb_queue_splice_tail_init(&tmpq, inputq);
-			__skb_dequeue(arrvq);
+			kfree_skb(__skb_dequeue(arrvq));
 		}
 		spin_unlock_bh(&inputq->lock);
 		__skb_queue_purge(&tmpq);
-- 
2.25.1

[tipc-discussion] [iproute2-next] tipc: call a sub-routine in separate socket

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

When receiving a result from first query to netlink, we may exec
a another query inside the callback. If calling this sub-routine
in the same socket, it will be discarded the result from previous
exection.
To avoid this we perform a nested query in separate socket.

Fixes: 202102830663 ("tipc: use the libmnl functions in lib/mnl_utils.c")
Signed-off-by: Hoang Le <hoa...@de...>
---
 tipc/link.c   | 15 +++++++++++++--
 tipc/socket.c | 17 +++++++++++++++--
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/tipc/link.c b/tipc/link.c
index 2123f109c694..9994ada2a367 100644
--- a/tipc/link.c
+++ b/tipc/link.c
@@ -17,7 +17,9 @@
 #include <linux/tipc_netlink.h>
 #include <linux/tipc.h>
 #include <linux/genetlink.h>
+#include <libmnl/libmnl.h>
 
+#include "mnl_utils.h"
 #include "cmdl.h"
 #include "msg.h"
 #include "link.h"
@@ -993,13 +995,20 @@ exit:
 
 static int link_mon_peer_list(uint32_t mon_ref)
 {
+	struct mnlu_gen_socket link_nlg;
 	struct nlmsghdr *nlh;
 	struct nlattr *nest;
 	int err = 0;
 
-	nlh = msg_init(TIPC_NL_MON_PEER_GET);
+	err = mnlu_gen_socket_open(&link_nlg, TIPC_GENL_V2_NAME,
+				   TIPC_GENL_V2_VERSION);
+	if (err)
+		return -1;
+	nlh = mnlu_gen_socket_cmd_prepare(&link_nlg, TIPC_NL_MON_PEER_GET,
+					  NLM_F_REQUEST | NLM_F_DUMP);
 	if (!nlh) {
 		fprintf(stderr, "error, message initialisation failed\n");
+		mnlu_gen_socket_close(&link_nlg);
 		return -1;
 	}
 
@@ -1007,7 +1016,9 @@ static int link_mon_peer_list(uint32_t mon_ref)
 	mnl_attr_put_u32(nlh, TIPC_NLA_MON_REF, mon_ref);
 	mnl_attr_nest_end(nlh, nest);
 
-	err = msg_dumpit(nlh, link_mon_peer_list_cb, NULL);
+	err = mnlu_gen_socket_sndrcv(&link_nlg, nlh, link_mon_peer_list_cb,
+				     NULL);
+	mnlu_gen_socket_close(&link_nlg);
 	return err;
 }
 
diff --git a/tipc/socket.c b/tipc/socket.c
index deae12af4409..597ffd91af52 100644
--- a/tipc/socket.c
+++ b/tipc/socket.c
@@ -15,7 +15,9 @@
 #include <linux/tipc.h>
 #include <linux/tipc_netlink.h>
 #include <linux/genetlink.h>
+#include <libmnl/libmnl.h>
 
+#include "mnl_utils.h"
 #include "cmdl.h"
 #include "msg.h"
 #include "socket.h"
@@ -44,12 +46,21 @@ static int publ_list_cb(const struct nlmsghdr *nlh, void *data)
 
 static int publ_list(uint32_t sock)
 {
+	struct mnlu_gen_socket sock_nlg;
 	struct nlmsghdr *nlh;
 	struct nlattr *nest;
+	int err;
 
-	nlh = msg_init(TIPC_NL_PUBL_GET);
+	err = mnlu_gen_socket_open(&sock_nlg, TIPC_GENL_V2_NAME,
+				   TIPC_GENL_V2_VERSION);
+	if (err)
+		return -1;
+
+	nlh = mnlu_gen_socket_cmd_prepare(&sock_nlg, TIPC_NL_PUBL_GET,
+					  NLM_F_REQUEST | NLM_F_DUMP);
 	if (!nlh) {
 		fprintf(stderr, "error, message initialisation failed\n");
+		mnlu_gen_socket_close(&sock_nlg);
 		return -1;
 	}
 
@@ -57,7 +68,9 @@ static int publ_list(uint32_t sock)
 	mnl_attr_put_u32(nlh, TIPC_NLA_SOCK_REF, sock);
 	mnl_attr_nest_end(nlh, nest);
 
-	return msg_dumpit(nlh, publ_list_cb, NULL);
+	err = mnlu_gen_socket_sndrcv(&sock_nlg, nlh, publ_list_cb, NULL);
+	mnlu_gen_socket_close(&sock_nlg);
+	return err;
 }
 
 static int sock_list_cb(const struct nlmsghdr *nlh, void *data)
-- 
2.25.1