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

Re: [tipc-discussion] tipc over an active/backup bond device

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

Hi Howard,
This is the code executed when TIPC receives a NETDEV_CHANGE event:

switch (evt) {
|       case NETDEV_CHANGE:
|       |       if (netif_carrier_ok(dev) && netif_oper_up(dev)) {
|       |       |       test_and_set_bit_lock(0, &b->up);
|       |       |       break;
|       |       }
|       |       fallthrough;
|       case NETDEV_GOING_DOWN:
|       |       clear_bit_unlock(0, &b->up);
|       |       tipc_reset_bearer(net, b);
|       |       break;
|       case NETDEV_UP:
|       |       test_and_set_bit_lock(0, &b->up);
|       |       break;
|       case NETDEV_CHANGEMTU:

So, unless the bond interface really reports that it is going down TIPC 
doesn't reset any links. And if it *does* report that it is going down, 
what else can we do?
To me this looks more like a problem with the bond device rather than 
with TIPC, but we might of course have misunderstood its expected behavior.
We will look into this.
On a different note, you could instead omit the bond interface and try 
using dual TIPC links, which work in active-active mode and give better 
performance.
Is that an option for you?

BR
Jon Maloy


On 11/19/20 11:36 PM, Howard Finer wrote:
> I am trying to use TIPC (kernel version 4.19) over a bond device that is
> configured for active-backup and arp monitoring for the slaves. If a slave
> goes down, TIPC is receiving a netdev_change during the timeframe that the
> bond device is working towards brining up the new slave.  This causes TIPC
> to disable the bearer, which in turn causes a temporary loss of
> communication between the nodes.
>
>   
>
> Instrumentation of the bond and tipc drivers shows the following:
>
> <6> 1 2020-11-19T23:58:33.111549+01:00 LABNBS5A kernel - - - [  153.655776]
> Enabled bearer <eth:bond0>, priority 10
>
> <6> 1 2020-11-20T00:07:58.544040+01:00 LABNBS5A kernel - - - [  718.799259]
> bond0: bond_ab_arp_commit: BOND_LINK_DOWN: link status definitely down for
> interface eth1, disabling it
>
> <6> 1 2020-11-20T00:07:58.544063+01:00 LABNBS5A kernel - - - [  718.799261]
> bond0: bond_ab_arp_commit: do_failover, block netpoll_tx and call
> select_active_slave
>
> <6> 1 2020-11-20T00:07:58.544069+01:00 LABNBS5A kernel - - - [  718.799263]
> bond0: bond_select_active_slave: bond_find_best_slave returned NULL
>
> <6> 1 2020-11-20T00:07:58.544072+01:00 LABNBS5A kernel - - - [  718.799347]
> bond0: bond_select_active_slave: now running without any active interface!
>
> <6> 1 2020-11-20T00:07:58.544080+01:00 LABNBS5A kernel - - - [  718.799349]
> bond0: bond_ab_arp_commit: do_failover, returned from select_active_slave
> and unblock netpoll tx
>
> <6> 1 2020-11-20T00:07:58.544081+01:00 LABNBS5A kernel - - - [  718.799611]
> Resetting bearer <eth:bond0>
>
> <6> 1 2020-11-20T00:07:58.655535+01:00 LABNBS5A kernel - - - [  718.907245]
> bond0: bond_ab_arp_commit: BOND_LINK_UP: link status definitely up for
> interface eth0
>
> <6> 1 2020-11-20T00:07:58.655545+01:00 LABNBS5A kernel - - - [  718.907247]
> bond0: bond_ab_arp_commit: do_failover, block netpoll_tx and call
> select_active_slave
>
> <6> 1 2020-11-20T00:07:58.655548+01:00 LABNBS5A kernel - - - [  718.907248]
> bond0: bond_select_active_slave: bond_find_best_slave returned slave eth0
>
> <6> 1 2020-11-20T00:07:58.655559+01:00 LABNBS5A kernel - - - [  718.907249]
> bond0: making interface eth0 the new active one
>
> <6> 1 2020-11-20T00:07:58.655562+01:00 LABNBS5A kernel - - - [  718.907560]
> bond0: bond_select_active_slave: first active interface up!
>
>   
>
> With arp based monitoring only 1 slave will be 'up'.  When the active slave
> goes down, the other slave needs to be brought up.  During that timeframe we
> see TIPC is resetting the bearer.    That defeats the entire purpose of
> using the bond device.
>
> It seems that the handling of the netdev_change event for a active/backup
> bond device is not correct.  It needs to leave the bearer intact so that
> when the backup slave is brought up the communication is properly restored
> without any upper layer applications being aware that something happened at
> the lower level.
>
>   
>
> Thanks,
>
> Howard
>
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>

[tipc-discussion] tipc over an active/backup bond device

[Howard F. <ho...@th...>]

I am trying to use TIPC (kernel version 4.19) over a bond device that is
configured for active-backup and arp monitoring for the slaves. If a slave
goes down, TIPC is receiving a netdev_change during the timeframe that the
bond device is working towards brining up the new slave.  This causes TIPC
to disable the bearer, which in turn causes a temporary loss of
communication between the nodes.

 

Instrumentation of the bond and tipc drivers shows the following:

<6> 1 2020-11-19T23:58:33.111549+01:00 LABNBS5A kernel - - - [  153.655776]
Enabled bearer <eth:bond0>, priority 10

<6> 1 2020-11-20T00:07:58.544040+01:00 LABNBS5A kernel - - - [  718.799259]
bond0: bond_ab_arp_commit: BOND_LINK_DOWN: link status definitely down for
interface eth1, disabling it

<6> 1 2020-11-20T00:07:58.544063+01:00 LABNBS5A kernel - - - [  718.799261]
bond0: bond_ab_arp_commit: do_failover, block netpoll_tx and call
select_active_slave

<6> 1 2020-11-20T00:07:58.544069+01:00 LABNBS5A kernel - - - [  718.799263]
bond0: bond_select_active_slave: bond_find_best_slave returned NULL

<6> 1 2020-11-20T00:07:58.544072+01:00 LABNBS5A kernel - - - [  718.799347]
bond0: bond_select_active_slave: now running without any active interface!

<6> 1 2020-11-20T00:07:58.544080+01:00 LABNBS5A kernel - - - [  718.799349]
bond0: bond_ab_arp_commit: do_failover, returned from select_active_slave
and unblock netpoll tx

<6> 1 2020-11-20T00:07:58.544081+01:00 LABNBS5A kernel - - - [  718.799611]
Resetting bearer <eth:bond0>

<6> 1 2020-11-20T00:07:58.655535+01:00 LABNBS5A kernel - - - [  718.907245]
bond0: bond_ab_arp_commit: BOND_LINK_UP: link status definitely up for
interface eth0

<6> 1 2020-11-20T00:07:58.655545+01:00 LABNBS5A kernel - - - [  718.907247]
bond0: bond_ab_arp_commit: do_failover, block netpoll_tx and call
select_active_slave

<6> 1 2020-11-20T00:07:58.655548+01:00 LABNBS5A kernel - - - [  718.907248]
bond0: bond_select_active_slave: bond_find_best_slave returned slave eth0

<6> 1 2020-11-20T00:07:58.655559+01:00 LABNBS5A kernel - - - [  718.907249]
bond0: making interface eth0 the new active one

<6> 1 2020-11-20T00:07:58.655562+01:00 LABNBS5A kernel - - - [  718.907560]
bond0: bond_select_active_slave: first active interface up!

 

With arp based monitoring only 1 slave will be 'up'.  When the active slave
goes down, the other slave needs to be brought up.  During that timeframe we
see TIPC is resetting the bearer.    That defeats the entire purpose of
using the bond device.

It seems that the handling of the netdev_change event for a active/backup
bond device is not correct.  It needs to leave the bearer intact so that
when the backup slave is brought up the communication is properly restored
without any upper layer applications being aware that something happened at
the lower level.

 

Thanks,

Howard

Re: [tipc-discussion] [net-next 0/3] tipc: some minor improvements

[Ying X. <yin...@wi...>]

On 11/12/20 9:26 AM, jm...@re... wrote:
> From: Jon Maloy <jm...@re...>
> 
> We add some improvements that will be useful in future commits.
> 

Acked-by: Ying Xue <yin...@wi...>

> Jon Maloy (3):
>   tipc: refactor tipc_sk_bind() function
>   tipc: make node number calculation reproducible
>   tipc: update address terminology in code
> 
>  net/tipc/addr.c       |   7 ++-
>  net/tipc/addr.h       |   1 +
>  net/tipc/core.h       |  14 ++++++
>  net/tipc/group.c      |   3 +-
>  net/tipc/group.h      |   3 +-
>  net/tipc/name_table.c |  11 +++--
>  net/tipc/net.c        |   2 +-
>  net/tipc/socket.c     | 110 ++++++++++++++++++++----------------------
>  net/tipc/subscr.c     |   5 +-
>  net/tipc/subscr.h     |   5 +-
>  net/tipc/topsrv.c     |   4 +-
>  11 files changed, 89 insertions(+), 76 deletions(-)
> 

Re: [tipc-discussion] [net-next 2/3] tipc: make node number calculation reproducible

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

On 11/13/20 7:23 AM, Ying Xue wrote:
> On 11/12/20 9:26 AM, jm...@re... wrote:
>> +static inline u32 hash128to32(char *bytes)
>> +{
>> +	u32 *tmp = (u32 *)bytes;
>> +	u32 res;
>> +
>> +	res = ntohl(tmp[0] ^ tmp[1] ^ tmp[2] ^ tmp[3]);
>> +	if (likely(res))
>> +		return res;
>> +	res = tmp[0] | tmp[1] | tmp[2] | tmp[3];
>> +	return !res ? 0 : ntohl(18140715);
> In case that the hashed result is accidentally equal to the fix
> number(ie, ntohl(18140715)), how would we be able to differentiate it
> with the case where the hashed result is 0?
We don't need to. As said in the commit log, we are always ready to handle
hash collisions.
See commit 25b0b9c4e835 ("tipc: handle collisions of 32-bit node address 
hash values")
We will need this function for my 128-bit addresses too, but even there 
hash
collisions will be anticipated and handled.

///jon
>> +}

Re: [tipc-discussion] [PATCH] tipc: fix -Wstringop-truncation warnings

[Ying X. <yin...@wi...>]

On 11/12/20 5:34 PM, Kang Wenlin wrote:
> From: Wenlin Kang <wen...@wi...>
> 
> Replace strncpy() with strscpy(), fixes the following warning:
> 
> In function 'bearer_name_validate',
>     inlined from 'tipc_enable_bearer' at net/tipc/bearer.c:246:7:
> net/tipc/bearer.c:141:2: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
>   strncpy(name_copy, name, TIPC_MAX_BEARER_NAME);
>   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Signed-off-by: Wenlin Kang <wen...@wi...>

Acked-by: Ying Xue <yin...@wi...>

> ---
>  net/tipc/bearer.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> index 650414110452..2241d5a38f7b 100644
> --- a/net/tipc/bearer.c
> +++ b/net/tipc/bearer.c
> @@ -139,10 +139,7 @@ static int bearer_name_validate(const char *name,
>  	u32 if_len;
>  
>  	/* copy bearer name & ensure length is OK */
> -	name_copy[TIPC_MAX_BEARER_NAME - 1] = 0;
> -	/* need above in case non-Posix strncpy() doesn't pad with nulls */
> -	strncpy(name_copy, name, TIPC_MAX_BEARER_NAME);
> -	if (name_copy[TIPC_MAX_BEARER_NAME - 1] != 0)
> +	if (strscpy(name_copy, name, TIPC_MAX_BEARER_NAME) < 0)
>  		return 0;
>  
>  	/* ensure all component parts of bearer name are present */
> 

Re: [tipc-discussion] [net-next 2/3] tipc: make node number calculation reproducible

[Ying X. <yin...@wi...>]

On 11/12/20 9:26 AM, jm...@re... wrote:
> +static inline u32 hash128to32(char *bytes)
> +{
> +	u32 *tmp = (u32 *)bytes;
> +	u32 res;
> +
> +	res = ntohl(tmp[0] ^ tmp[1] ^ tmp[2] ^ tmp[3]);
> +	if (likely(res))
> +		return res;
> +	res = tmp[0] | tmp[1] | tmp[2] | tmp[3];
> +	return !res ? 0 : ntohl(18140715);

In case that the hashed result is accidentally equal to the fix
number(ie, ntohl(18140715)), how would we be able to differentiate it
with the case where the hashed result is 0?

> +}

[tipc-discussion] [net-next 3/3] tipc: update address terminology in code

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

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

We update the terminology in the code so that deprecated structure
names and macros are replaced with those currently recommended in
the user API.

struct tipc_portid   -> struct tipc_socket_addr
struct tipc_name     -> struct tipc_service_addr
struct tipc_name_seq -> struct tipc_service_range

TIPC_ADDR_ID       -> TIPC_SOCKET_ADDR
TIPC_ADDR_NAME     -> TIPC_SERVICE_ADDR
TIPC_ADDR_NAMESEQ  -> TIPC_SERVICE_RANGE
TIPC_CFG_SRV       -> TIPC_NODE_STATE

Signed-off-by: Jon Maloy <jm...@re...>
---
 net/tipc/group.c      |  3 ++-
 net/tipc/group.h      |  3 ++-
 net/tipc/name_table.c | 11 ++++++-----
 net/tipc/net.c        |  2 +-
 net/tipc/socket.c     | 44 +++++++++++++++++++++----------------------
 net/tipc/subscr.c     |  5 +++--
 net/tipc/subscr.h     |  5 +++--
 net/tipc/topsrv.c     |  4 ++--
 8 files changed, 41 insertions(+), 36 deletions(-)

diff --git a/net/tipc/group.c b/net/tipc/group.c
index b1fcd2ad5ecf..3e137d8c9d2f 100644
--- a/net/tipc/group.c
+++ b/net/tipc/group.c
@@ -2,6 +2,7 @@
  * net/tipc/group.c: TIPC group messaging code
  *
  * Copyright (c) 2017, Ericsson AB
+ * Copyright (c) 2020, Red Hat Inc
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -359,7 +360,7 @@ struct tipc_nlist *tipc_group_dests(struct tipc_group *grp)
 	return &grp->dests;
 }
 
-void tipc_group_self(struct tipc_group *grp, struct tipc_name_seq *seq,
+void tipc_group_self(struct tipc_group *grp, struct tipc_service_range *seq,
 		     int *scope)
 {
 	seq->type = grp->type;
diff --git a/net/tipc/group.h b/net/tipc/group.h
index 76b4e5a7b39d..ea4c3be64c78 100644
--- a/net/tipc/group.h
+++ b/net/tipc/group.h
@@ -2,6 +2,7 @@
  * net/tipc/group.h: Include file for TIPC group unicast/multicast functions
  *
  * Copyright (c) 2017, Ericsson AB
+ * Copyright (c) 2020, Red Hat Inc
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -50,7 +51,7 @@ void tipc_group_delete(struct net *net, struct tipc_group *grp);
 void tipc_group_add_member(struct tipc_group *grp, u32 node,
 			   u32 port, u32 instance);
 struct tipc_nlist *tipc_group_dests(struct tipc_group *grp);
-void tipc_group_self(struct tipc_group *grp, struct tipc_name_seq *seq,
+void tipc_group_self(struct tipc_group *grp, struct tipc_service_range *seq,
 		     int *scope);
 u32 tipc_group_exclude(struct tipc_group *grp);
 void tipc_group_filter_msg(struct tipc_group *grp,
diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
index 2ac33d32edc2..e1233d6d5163 100644
--- a/net/tipc/name_table.c
+++ b/net/tipc/name_table.c
@@ -3,6 +3,7 @@
  *
  * Copyright (c) 2000-2006, 2014-2018, Ericsson AB
  * Copyright (c) 2004-2008, 2010-2014, Wind River Systems
+ * Copyright (c) 2020, Red Hat Inc
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -403,12 +404,12 @@ static void tipc_service_subscribe(struct tipc_service *service,
 	struct publication *p, *first, *tmp;
 	struct list_head publ_list;
 	struct service_range *sr;
-	struct tipc_name_seq ns;
+	struct tipc_service_range r;
 	u32 filter;
 
-	ns.type = tipc_sub_read(sb, seq.type);
-	ns.lower = tipc_sub_read(sb, seq.lower);
-	ns.upper = tipc_sub_read(sb, seq.upper);
+	r.type = tipc_sub_read(sb, seq.type);
+	r.lower = tipc_sub_read(sb, seq.lower);
+	r.upper = tipc_sub_read(sb, seq.upper);
 	filter = tipc_sub_read(sb, filter);
 
 	tipc_sub_get(sub);
@@ -418,7 +419,7 @@ static void tipc_service_subscribe(struct tipc_service *service,
 		return;
 
 	INIT_LIST_HEAD(&publ_list);
-	service_range_foreach_match(sr, service, ns.lower, ns.upper) {
+	service_range_foreach_match(sr, service, r.lower, r.upper) {
 		first = NULL;
 		list_for_each_entry(p, &sr->all_publ, all_publ) {
 			if (filter & TIPC_SUB_PORTS)
diff --git a/net/tipc/net.c b/net/tipc/net.c
index 0bb2323201da..a129f661bee3 100644
--- a/net/tipc/net.c
+++ b/net/tipc/net.c
@@ -132,7 +132,7 @@ static void tipc_net_finalize(struct net *net, u32 addr)
 	tipc_named_reinit(net);
 	tipc_sk_reinit(net);
 	tipc_mon_reinit_self(net);
-	tipc_nametbl_publish(net, TIPC_CFG_SRV, addr, addr,
+	tipc_nametbl_publish(net, TIPC_NODE_STATE, addr, addr,
 			     TIPC_CLUSTER_SCOPE, 0, addr);
 }
 
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 2b633463f40d..75e81fc8e9a8 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -139,9 +139,9 @@ static int tipc_accept(struct socket *sock, struct socket *new_sock, int flags,
 		       bool kern);
 static void tipc_sk_timeout(struct timer_list *t);
 static int tipc_sk_publish(struct tipc_sock *tsk, uint scope,
-			   struct tipc_name_seq const *seq);
+			   struct tipc_service_range const *seq);
 static int tipc_sk_withdraw(struct tipc_sock *tsk, uint scope,
-			    struct tipc_name_seq const *seq);
+			    struct tipc_service_range const *seq);
 static int tipc_sk_leave(struct tipc_sock *tsk);
 static struct tipc_sock *tipc_sk_lookup(struct net *net, u32 portid);
 static int tipc_sk_insert(struct tipc_sock *tsk);
@@ -667,7 +667,7 @@ static int __tipc_bind(struct socket *sock, struct sockaddr *skaddr, int alen)
 	if (unlikely(!alen))
 		return tipc_sk_withdraw(tsk, 0, NULL);
 
-	if (addr->addrtype == TIPC_ADDR_NAME)
+	if (addr->addrtype == TIPC_SERVICE_ADDR)
 		addr->addr.nameseq.upper = addr->addr.nameseq.lower;
 
 	if (tsk->group)
@@ -740,7 +740,7 @@ static int tipc_getname(struct socket *sock, struct sockaddr *uaddr,
 		addr->addr.id.node = tipc_own_addr(sock_net(sk));
 	}
 
-	addr->addrtype = TIPC_ADDR_ID;
+	addr->addrtype = TIPC_SOCKET_ADDR;
 	addr->family = AF_TIPC;
 	addr->scope = 0;
 	addr->addr.name.domain = 0;
@@ -818,7 +818,7 @@ static __poll_t tipc_poll(struct file *file, struct socket *sock,
  * Called from function tipc_sendmsg(), which has done all sanity checks
  * Returns the number of bytes sent on success, or errno
  */
-static int tipc_sendmcast(struct  socket *sock, struct tipc_name_seq *seq,
+static int tipc_sendmcast(struct  socket *sock, struct tipc_service_range *seq,
 			  struct msghdr *msg, size_t dlen, long timeout)
 {
 	struct sock *sk = sock->sk;
@@ -1403,7 +1403,7 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
 	bool syn = !tipc_sk_type_connectionless(sk);
 	struct tipc_group *grp = tsk->group;
 	struct tipc_msg *hdr = &tsk->phdr;
-	struct tipc_name_seq *seq;
+	struct tipc_service_range *seq;
 	struct sk_buff_head pkts;
 	u32 dport = 0, dnode = 0;
 	u32 type = 0, inst = 0;
@@ -1422,9 +1422,9 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
 	if (grp) {
 		if (!dest)
 			return tipc_send_group_bcast(sock, m, dlen, timeout);
-		if (dest->addrtype == TIPC_ADDR_NAME)
+		if (dest->addrtype == TIPC_SERVICE_ADDR)
 			return tipc_send_group_anycast(sock, m, dlen, timeout);
-		if (dest->addrtype == TIPC_ADDR_ID)
+		if (dest->addrtype == TIPC_SOCKET_ADDR)
 			return tipc_send_group_unicast(sock, m, dlen, timeout);
 		if (dest->addrtype == TIPC_ADDR_MCAST)
 			return tipc_send_group_mcast(sock, m, dlen, timeout);
@@ -1444,7 +1444,7 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
 			return -EISCONN;
 		if (tsk->published)
 			return -EOPNOTSUPP;
-		if (dest->addrtype == TIPC_ADDR_NAME) {
+		if (dest->addrtype == TIPC_SERVICE_ADDR) {
 			tsk->conn_type = dest->addr.name.name.type;
 			tsk->conn_instance = dest->addr.name.name.instance;
 		}
@@ -1455,14 +1455,14 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
 	if (dest->addrtype == TIPC_ADDR_MCAST)
 		return tipc_sendmcast(sock, seq, m, dlen, timeout);
 
-	if (dest->addrtype == TIPC_ADDR_NAME) {
+	if (dest->addrtype == TIPC_SERVICE_ADDR) {
 		type = dest->addr.name.name.type;
 		inst = dest->addr.name.name.instance;
 		dnode = dest->addr.name.domain;
 		dport = tipc_nametbl_translate(net, type, inst, &dnode);
 		if (unlikely(!dport && !dnode))
 			return -EHOSTUNREACH;
-	} else if (dest->addrtype == TIPC_ADDR_ID) {
+	} else if (dest->addrtype == TIPC_SOCKET_ADDR) {
 		dnode = dest->addr.id.node;
 	} else {
 		return -EINVAL;
@@ -1474,7 +1474,7 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
 	if (unlikely(rc))
 		return rc;
 
-	if (dest->addrtype == TIPC_ADDR_NAME) {
+	if (dest->addrtype == TIPC_SERVICE_ADDR) {
 		msg_set_type(hdr, TIPC_NAMED_MSG);
 		msg_set_hdr_sz(hdr, NAMED_H_SIZE);
 		msg_set_nametype(hdr, type);
@@ -1482,7 +1482,7 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
 		msg_set_lookup_scope(hdr, tipc_node2scope(dnode));
 		msg_set_destnode(hdr, dnode);
 		msg_set_destport(hdr, dport);
-	} else { /* TIPC_ADDR_ID */
+	} else { /* TIPC_SOCKET_ADDR */
 		msg_set_type(hdr, TIPC_DIRECT_MSG);
 		msg_set_lookup_scope(hdr, 0);
 		msg_set_destnode(hdr, dnode);
@@ -1687,7 +1687,7 @@ static void tipc_sk_set_orig_addr(struct msghdr *m, struct sk_buff *skb)
 		return;
 
 	srcaddr->sock.family = AF_TIPC;
-	srcaddr->sock.addrtype = TIPC_ADDR_ID;
+	srcaddr->sock.addrtype = TIPC_SOCKET_ADDR;
 	srcaddr->sock.scope = 0;
 	srcaddr->sock.addr.id.ref = msg_origport(hdr);
 	srcaddr->sock.addr.id.node = msg_orignode(hdr);
@@ -1699,7 +1699,7 @@ static void tipc_sk_set_orig_addr(struct msghdr *m, struct sk_buff *skb)
 
 	/* Group message users may also want to know sending member's id */
 	srcaddr->member.family = AF_TIPC;
-	srcaddr->member.addrtype = TIPC_ADDR_NAME;
+	srcaddr->member.addrtype = TIPC_SERVICE_ADDR;
 	srcaddr->member.scope = 0;
 	srcaddr->member.addr.name.name.type = msg_nametype(hdr);
 	srcaddr->member.addr.name.name.instance = TIPC_SKB_CB(skb)->orig_member;
@@ -2867,7 +2867,7 @@ static void tipc_sk_timeout(struct timer_list *t)
 }
 
 static int tipc_sk_publish(struct tipc_sock *tsk, uint scope,
-			   struct tipc_name_seq const *seq)
+			   struct tipc_service_range const *seq)
 {
 	struct sock *sk = &tsk->sk;
 	struct net *net = sock_net(sk);
@@ -2895,7 +2895,7 @@ static int tipc_sk_publish(struct tipc_sock *tsk, uint scope,
 }
 
 static int tipc_sk_withdraw(struct tipc_sock *tsk, uint scope,
-			    struct tipc_name_seq const *seq)
+			    struct tipc_service_range const *seq)
 {
 	struct net *net = sock_net(&tsk->sk);
 	struct publication *publ;
@@ -3042,7 +3042,7 @@ static int tipc_sk_join(struct tipc_sock *tsk, struct tipc_group_req *mreq)
 	struct net *net = sock_net(&tsk->sk);
 	struct tipc_group *grp = tsk->group;
 	struct tipc_msg *hdr = &tsk->phdr;
-	struct tipc_name_seq seq;
+	struct tipc_service_range seq;
 	int rc;
 
 	if (mreq->type < TIPC_RESERVED_TYPES)
@@ -3079,7 +3079,7 @@ static int tipc_sk_leave(struct tipc_sock *tsk)
 {
 	struct net *net = sock_net(&tsk->sk);
 	struct tipc_group *grp = tsk->group;
-	struct tipc_name_seq seq;
+	struct tipc_service_range seq;
 	int scope;
 
 	if (!grp)
@@ -3203,7 +3203,7 @@ static int tipc_getsockopt(struct socket *sock, int lvl, int opt,
 {
 	struct sock *sk = sock->sk;
 	struct tipc_sock *tsk = tipc_sk(sk);
-	struct tipc_name_seq seq;
+	struct tipc_service_range seq;
 	int len, scope;
 	u32 value;
 	int res;
@@ -3304,12 +3304,12 @@ static int tipc_socketpair(struct socket *sock1, struct socket *sock2)
 	u32 onode = tipc_own_addr(sock_net(sock1->sk));
 
 	tsk1->peer.family = AF_TIPC;
-	tsk1->peer.addrtype = TIPC_ADDR_ID;
+	tsk1->peer.addrtype = TIPC_SOCKET_ADDR;
 	tsk1->peer.scope = TIPC_NODE_SCOPE;
 	tsk1->peer.addr.id.ref = tsk2->portid;
 	tsk1->peer.addr.id.node = onode;
 	tsk2->peer.family = AF_TIPC;
-	tsk2->peer.addrtype = TIPC_ADDR_ID;
+	tsk2->peer.addrtype = TIPC_SOCKET_ADDR;
 	tsk2->peer.scope = TIPC_NODE_SCOPE;
 	tsk2->peer.addr.id.ref = tsk1->portid;
 	tsk2->peer.addr.id.node = onode;
diff --git a/net/tipc/subscr.c b/net/tipc/subscr.c
index f340e53da625..5edfb2d522b9 100644
--- a/net/tipc/subscr.c
+++ b/net/tipc/subscr.c
@@ -3,6 +3,7 @@
  *
  * Copyright (c) 2000-2017, Ericsson AB
  * Copyright (c) 2005-2007, 2010-2013, Wind River Systems
+ * Copyright (c) 2020, Red Hat Inc
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -60,7 +61,7 @@ static void tipc_sub_send_event(struct tipc_subscription *sub,
  *
  * Returns 1 if there is overlap, otherwise 0.
  */
-int tipc_sub_check_overlap(struct tipc_name_seq *seq, u32 found_lower,
+int tipc_sub_check_overlap(struct tipc_service_range *seq, u32 found_lower,
 			   u32 found_upper)
 {
 	if (found_lower < seq->lower)
@@ -79,7 +80,7 @@ void tipc_sub_report_overlap(struct tipc_subscription *sub,
 {
 	struct tipc_subscr *s = &sub->evt.s;
 	u32 filter = tipc_sub_read(s, filter);
-	struct tipc_name_seq seq;
+	struct tipc_service_range seq;
 
 	seq.type = tipc_sub_read(s, seq.type);
 	seq.lower = tipc_sub_read(s, seq.lower);
diff --git a/net/tipc/subscr.h b/net/tipc/subscr.h
index 6ebbec1bedd1..a083b1b0c1d2 100644
--- a/net/tipc/subscr.h
+++ b/net/tipc/subscr.h
@@ -3,6 +3,7 @@
  *
  * Copyright (c) 2003-2017, Ericsson AB
  * Copyright (c) 2005-2007, 2012-2013, Wind River Systems
+ * Copyright (c) 2020, Red Hat Inc
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -71,8 +72,8 @@ struct tipc_subscription *tipc_sub_subscribe(struct net *net,
 					     int conid);
 void tipc_sub_unsubscribe(struct tipc_subscription *sub);
 
-int tipc_sub_check_overlap(struct tipc_name_seq *seq, u32 found_lower,
-			   u32 found_upper);
+int tipc_sub_check_overlap(struct tipc_service_range *seq,
+			   u32 found_lower, u32 found_upper);
 void tipc_sub_report_overlap(struct tipc_subscription *sub,
 			     u32 found_lower, u32 found_upper,
 			     u32 event, u32 port, u32 node,
diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index cec029349662..69a994871dd7 100644
--- a/net/tipc/topsrv.c
+++ b/net/tipc/topsrv.c
@@ -519,8 +519,8 @@ static int tipc_topsrv_create_listener(struct tipc_topsrv *srv)
 		goto err;
 
 	saddr.family	                = AF_TIPC;
-	saddr.addrtype		        = TIPC_ADDR_NAMESEQ;
-	saddr.addr.nameseq.type         = TIPC_TOP_SRV;
+	saddr.addrtype		        = TIPC_SERVICE_RANGE;
+	saddr.addr.nameseq.type	= TIPC_TOP_SRV;
 	saddr.addr.nameseq.lower	= TIPC_TOP_SRV;
 	saddr.addr.nameseq.upper	= TIPC_TOP_SRV;
 	saddr.scope			= TIPC_NODE_SCOPE;
-- 
2.25.4

[tipc-discussion] [net-next 2/3] tipc: make node number calculation reproducible

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

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

The 32-bit node number, aka node hash or node address, is calculated
based on the 128-bit node identity when it is not set explicitly by
the user. In future commits we will need to perform this hash operation
on peer nodes while feeling safe that we obtain the same result.

We do this by interpreting the initial hash as a network byte order
number. Whenever we need to use the number locally on a node
we must therefore translate it to host byte order to obtain an
architecure independent result.

Furthermore, given the context where we use this number, we must not
allow it to be zero unless the node identity also is zero. Hence, in
the rare cases when the xor-ed hash value may end up as zero we replace
it with a fix number, knowing that the code anyway is capable of
handling hash collisions.

Signed-off-by: Jon Maloy <jm...@re...>
---
 net/tipc/addr.c |  7 +++----
 net/tipc/addr.h |  1 +
 net/tipc/core.h | 13 +++++++++++++
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/net/tipc/addr.c b/net/tipc/addr.c
index 0f1eaed1bd1b..abe29d1aa23a 100644
--- a/net/tipc/addr.c
+++ b/net/tipc/addr.c
@@ -55,12 +55,11 @@ bool tipc_in_scope(bool legacy_format, u32 domain, u32 addr)
 void tipc_set_node_id(struct net *net, u8 *id)
 {
 	struct tipc_net *tn = tipc_net(net);
-	u32 *tmp = (u32 *)id;
 
 	memcpy(tn->node_id, id, NODE_ID_LEN);
 	tipc_nodeid2string(tn->node_id_string, id);
-	tn->trial_addr = tmp[0] ^ tmp[1] ^ tmp[2] ^ tmp[3];
-	pr_info("Own node identity %s, cluster identity %u\n",
+	tn->trial_addr = hash128to32(id);
+	pr_info("Node identity %s, cluster identity %u\n",
 		tipc_own_id_string(net), tn->net_id);
 }
 
@@ -76,7 +75,7 @@ void tipc_set_node_addr(struct net *net, u32 addr)
 	}
 	tn->trial_addr = addr;
 	tn->addr_trial_end = jiffies;
-	pr_info("32-bit node address hash set to %x\n", addr);
+	pr_info("Node number set to %u\n", addr);
 }
 
 char *tipc_nodeid2string(char *str, u8 *id)
diff --git a/net/tipc/addr.h b/net/tipc/addr.h
index 31bee0ea7b3e..1a11831bef62 100644
--- a/net/tipc/addr.h
+++ b/net/tipc/addr.h
@@ -3,6 +3,7 @@
  *
  * Copyright (c) 2000-2006, 2018, Ericsson AB
  * Copyright (c) 2004-2005, Wind River Systems
+ * Copyright (c) 2020, Red Hat Inc
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
diff --git a/net/tipc/core.h b/net/tipc/core.h
index df34dcdd0607..464ad20dd2cc 100644
--- a/net/tipc/core.h
+++ b/net/tipc/core.h
@@ -3,6 +3,7 @@
  *
  * Copyright (c) 2005-2006, 2013-2018 Ericsson AB
  * Copyright (c) 2005-2007, 2010-2013, Wind River Systems
+ * Copyright (c) 2020, Red Hat Inc
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -210,6 +211,18 @@ static inline u32 tipc_net_hash_mixes(struct net *net, int tn_rand)
 	return net_hash_mix(&init_net) ^ net_hash_mix(net) ^ tn_rand;
 }
 
+static inline u32 hash128to32(char *bytes)
+{
+	u32 *tmp = (u32 *)bytes;
+	u32 res;
+
+	res = ntohl(tmp[0] ^ tmp[1] ^ tmp[2] ^ tmp[3]);
+	if (likely(res))
+		return res;
+	res = tmp[0] | tmp[1] | tmp[2] | tmp[3];
+	return !res ? 0 : ntohl(18140715);
+}
+
 #ifdef CONFIG_SYSCTL
 int tipc_register_sysctl(void);
 void tipc_unregister_sysctl(void);
-- 
2.25.4

[tipc-discussion] [net-next 0/3] tipc: some minor improvements

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

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

We add some improvements that will be useful in future commits.

Jon Maloy (3):
  tipc: refactor tipc_sk_bind() function
  tipc: make node number calculation reproducible
  tipc: update address terminology in code

 net/tipc/addr.c       |   7 ++-
 net/tipc/addr.h       |   1 +
 net/tipc/core.h       |  14 ++++++
 net/tipc/group.c      |   3 +-
 net/tipc/group.h      |   3 +-
 net/tipc/name_table.c |  11 +++--
 net/tipc/net.c        |   2 +-
 net/tipc/socket.c     | 110 ++++++++++++++++++++----------------------
 net/tipc/subscr.c     |   5 +-
 net/tipc/subscr.h     |   5 +-
 net/tipc/topsrv.c     |   4 +-
 11 files changed, 89 insertions(+), 76 deletions(-)

-- 
2.25.4

[tipc-discussion] [net-next 1/3] tipc: refactor tipc_sk_bind() function

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

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

We refactor the tipc_sk_bind() function, so that the lock handling
is handled separately from the logics. We also move some sanity
tests to earlier in the call chain, to the function tipc_bind().

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

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 69c4b16e8184..2b633463f40d 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1,8 +1,9 @@
 /*
  * net/tipc/socket.c: TIPC socket API
  *
- * Copyright (c) 2001-2007, 2012-2017, Ericsson AB
+ * Copyright (c) 2001-2007, 2012-2019, Ericsson AB
  * Copyright (c) 2004-2008, 2010-2013, Wind River Systems
+ * Copyright (c) 2020, Red Hat Inc
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -644,10 +645,10 @@ static int tipc_release(struct socket *sock)
 }
 
 /**
- * tipc_bind - associate or disassocate TIPC name(s) with a socket
+ * __tipc_bind - associate or disassocate TIPC name(s) with a socket
  * @sock: socket structure
- * @uaddr: socket address describing name(s) and desired operation
- * @uaddr_len: size of socket address data structure
+ * @skaddr: socket address describing name(s) and desired operation
+ * @alen: size of socket address data structure
  *
  * Name and name sequence binding is indicated using a positive scope value;
  * a negative scope value unbinds the specified name.  Specifying no name
@@ -658,44 +659,33 @@ static int tipc_release(struct socket *sock)
  * NOTE: This routine doesn't need to take the socket lock since it doesn't
  *       access any non-constant socket information.
  */
-
-int tipc_sk_bind(struct socket *sock, struct sockaddr *uaddr, int uaddr_len)
+static int __tipc_bind(struct socket *sock, struct sockaddr *skaddr, int alen)
 {
-	struct sock *sk = sock->sk;
-	struct sockaddr_tipc *addr = (struct sockaddr_tipc *)uaddr;
-	struct tipc_sock *tsk = tipc_sk(sk);
-	int res = -EINVAL;
+	struct sockaddr_tipc *addr = (struct sockaddr_tipc *)skaddr;
+	struct tipc_sock *tsk = tipc_sk(sock->sk);
 
-	lock_sock(sk);
-	if (unlikely(!uaddr_len)) {
-		res = tipc_sk_withdraw(tsk, 0, NULL);
-		goto exit;
-	}
-	if (tsk->group) {
-		res = -EACCES;
-		goto exit;
-	}
-	if (uaddr_len < sizeof(struct sockaddr_tipc)) {
-		res = -EINVAL;
-		goto exit;
-	}
-	if (addr->family != AF_TIPC) {
-		res = -EAFNOSUPPORT;
-		goto exit;
-	}
+	if (unlikely(!alen))
+		return tipc_sk_withdraw(tsk, 0, NULL);
 
 	if (addr->addrtype == TIPC_ADDR_NAME)
 		addr->addr.nameseq.upper = addr->addr.nameseq.lower;
-	else if (addr->addrtype != TIPC_ADDR_NAMESEQ) {
-		res = -EAFNOSUPPORT;
-		goto exit;
-	}
 
-	res = (addr->scope >= 0) ?
-		tipc_sk_publish(tsk, addr->scope, &addr->addr.nameseq) :
-		tipc_sk_withdraw(tsk, -addr->scope, &addr->addr.nameseq);
-exit:
-	release_sock(sk);
+	if (tsk->group)
+		return -EACCES;
+
+	if (addr->scope >= 0)
+		return tipc_sk_publish(tsk, addr->scope, &addr->addr.nameseq);
+	else
+		return tipc_sk_withdraw(tsk, -addr->scope, &addr->addr.nameseq);
+}
+
+int tipc_sk_bind(struct socket *sock, struct sockaddr *skaddr, int alen)
+{
+	int res;
+
+	lock_sock(sock->sk);
+	res = __tipc_bind(sock, skaddr, alen);
+	release_sock(sock->sk);
 	return res;
 }
 
@@ -706,6 +696,10 @@ static int tipc_bind(struct socket *sock, struct sockaddr *skaddr, int alen)
 	if (alen) {
 		if (alen < sizeof(struct sockaddr_tipc))
 			return -EINVAL;
+		if (addr->family != AF_TIPC)
+			return -EAFNOSUPPORT;
+		if (addr->addrtype > TIPC_SERVICE_ADDR)
+			return -EAFNOSUPPORT;
 		if (addr->addr.nameseq.type < TIPC_RESERVED_TYPES) {
 			pr_warn_once("Can't bind to reserved service type %u\n",
 				     addr->addr.nameseq.type);
-- 
2.25.4

Re: [tipc-discussion] [net v3 1/1] tipc: fix memory leak caused by tipc_buf_append()

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

Hi Cong,

No, I have never ignored any comment from reviewers. I sent v2 on Oct 26 after discussing with Xin Long, and v3 on Oct 27 after receiving comment from Jakub.
I received your 3 emails nearly at the same time on Oct 28. It's weird. Your emails did not appear in this email archive either: https://sourceforge.net/p/tipc/mailman/tipc-discussion/

Anyway, I answer your questions:
1/ Why it is not correct if not decreasing the data reference counter in tipc_buf_append()
In my changelog, I just pinpointed the place where the leak would happen. I show you the details here:
tipc_msg_reassemble(list,-)
{
 ...
 frag = skb_clone(skb, GFP_ATOMIC); // each data reference counter of the original skb has the value of 2.
 ...
 If (tipc_buf_append(&head, &frag)) // each data reference counter of the original skb STILL has the value of 2 because the usage of skb_copy() instead of skb_unshare()
 ...
}
The original skb list then is passed to tipc_bcast_xmit() which in turn calls tipc_link_xmit():
tipc_link_xmit(-, list, -)
{
 ...
 _skb = skb_clone(skb, GFP_ATOMIC); // each data reference counter of the original skb has the value of 3.
...
}

When each cloned skb is sent out by the driver, it is freed by the driver. That leads to each data reference counter of the original skb has the value of 2.
After receiving ACK from another peer, the original skb needs to be freed:
tipc_link_advance_transmq()
{
 ...
 kfree_skb(skb);  // memory exists after being freed because the data reference counter still has the value of 2.
}

This indeed causes memory leak.

2/ Why previously-used skb_unclone() works.
The purpose of skb_unclone() is to unclone the cloned skb. So, it does not make any sense to say that " skb_unclone() expects refcnt == 1" as I understand
you implied the data reference counter.
pskb_expand_head() inside skb_unclone() requires that the user reference counter has the value of 1 as implemented:
pskb_expand_head()
{
 ...
 BUG_ON(skb_shared(skb)); // User reference counter must be 1
...
atomic_set(&skb_shinfo(skb)->dataref, 1); // The data reference counter of the original skb has the value of 1
...
}
That explains why after being passed to tipc_link_xmit(), each data reference counter of each original skb has the value of 2 and can be freed in tipc_link_advance_transmq().

Best regards,
Tung Nguyen

-----Original Message-----
From: Cong Wang <xiy...@gm...> 
Sent: Wednesday, October 28, 2020 3:50 AM
To: Tung Quang Nguyen <tun...@de...>
Cc: David Miller <da...@da...>; Linux Kernel Network Developers <ne...@vg...>; tip...@li...
Subject: Re: [tipc-discussion] [net v3 1/1] tipc: fix memory leak caused by tipc_buf_append()

On Tue, Oct 27, 2020 at 1:09 PM Tung Nguyen
<tun...@de...> wrote:
>
> Commit ed42989eab57 ("tipc: fix the skb_unshare() in tipc_buf_append()")
> replaced skb_unshare() with skb_copy() to not reduce the data reference
> counter of the original skb intentionally. This is not the correct
> way to handle the cloned skb because it causes memory leak in 2
> following cases:
>  1/ Sending multicast messages via broadcast link
>   The original skb list is cloned to the local skb list for local
>   destination. After that, the data reference counter of each skb
>   in the original list has the value of 2. This causes each skb not
>   to be freed after receiving ACK:

This does not make sense at all.

skb_unclone() expects refcnt == 1, as stated in the comments
above pskb_expand_head(). skb_unclone() was used prior to
Xin Long's commit.

So either the above is wrong, or something important is still missing
in your changelog. None of them is addressed in your V3.

I also asked you two questions before you sent V3, you seem to
intentionally ignore them. This is not how we collaborate.

Thanks.

Re: [tipc-discussion] [net v3 1/1] tipc: fix memory leak caused by tipc_buf_append()

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

On Tue, Oct 27, 2020 at 11:25 AM Tung Nguyen
<tun...@de...> wrote:
>
> Commit ed42989eab57 ("tipc: fix the skb_unshare() in tipc_buf_append()")
> replaced skb_unshare() with skb_copy() to not reduce the data reference
> counter of the original skb intentionally. This is not the correct
> way to handle the cloned skb because it causes memory leak in 2
> following cases:
>  1/ Sending multicast messages via broadcast link
>   The original skb list is cloned to the local skb list for local
>   destination. After that, the data reference counter of each skb
>   in the original list has the value of 2. This causes each skb not
>   to be freed after receiving ACK:
>   tipc_link_advance_transmq()
>   {
>    ...
>    /* release skb */
>    __skb_unlink(skb, &l->transmq);
>    kfree_skb(skb); <-- memory exists after being freed
>   }
>
>  2/ Sending multicast messages via replicast link
>   Similar to the above case, each skb cannot be freed after purging
>   the skb list:
>   tipc_mcast_xmit()
>   {
>    ...
>    __skb_queue_purge(pkts); <-- memory exists after being freed
>   }
>
> This commit fixes this issue by using skb_unshare() instead. Besides,
> to avoid use-after-free error reported by KASAN, the pointer to the
> fragment is set to NULL before calling skb_unshare() to make sure that
> the original skb is not freed after freeing the fragment 2 times in
> case skb_unshare() returns NULL.
>
> Fixes: ed42989eab57 ("tipc: fix the skb_unshare() in tipc_buf_append()")
> Acked-by: Jon Maloy <jm...@re...>
> Reported-by: Thang Hoang Ngo <tha...@de...>
> Signed-off-by: Tung Nguyen <tun...@de...>
Reviewed-by: Xin Long <luc...@gm...>

> ---
>  net/tipc/msg.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index 2a78aa701572..32c79c59052b 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -150,12 +150,11 @@ int tipc_buf_append(struct sk_buff **headbuf, struct sk_buff **buf)
>         if (fragid == FIRST_FRAGMENT) {
>                 if (unlikely(head))
>                         goto err;
> -               if (skb_cloned(frag))
> -                       frag = skb_copy(frag, GFP_ATOMIC);
> +               *buf = NULL;
> +               frag = skb_unshare(frag, GFP_ATOMIC);
>                 if (unlikely(!frag))
>                         goto err;
>                 head = *headbuf = frag;
> -               *buf = NULL;
>                 TIPC_SKB_CB(head)->tail = NULL;
>                 if (skb_is_nonlinear(head)) {
>                         skb_walk_frags(head, tail) {
> --
> 2.17.1
>
>
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion

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

[Ying X. <yin...@wi...>]

On 10/27/20 7:07 AM, jm...@re... wrote:
> 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 reserved types which were really
> in use ([0,1]), and block all the rest ([2,63]).
> 
> This is risky, 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., most 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 also be made to bypass the checks in tipc_bind()
> by introducing a different entry function, tipc_sk_bind().
> 
> 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...>

Acked-by: Ying Xue <yin...@wi...>

> ---
>  net/tipc/socket.c | 24 +++++++++++++++---------
>  net/tipc/socket.h |  2 +-
>  net/tipc/topsrv.c |  4 ++--
>  3 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index e795a8a2955b..222fd53da2d0 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -658,8 +658,8 @@ static int tipc_release(struct socket *sock)
>   * NOTE: This routine doesn't need to take the socket lock since it doesn't
>   *       access any non-constant socket information.
>   */
> -static int tipc_bind(struct socket *sock, struct sockaddr *uaddr,
> -		     int uaddr_len)
> +
> +int tipc_sk_bind(struct socket *sock, struct sockaddr *uaddr, int uaddr_len)
>  {
>  	struct sock *sk = sock->sk;
>  	struct sockaddr_tipc *addr = (struct sockaddr_tipc *)uaddr;
> @@ -691,13 +691,6 @@ 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)) {
> -		res = -EACCES;
> -		goto exit;
> -	}
> -
>  	res = (addr->scope >= 0) ?
>  		tipc_sk_publish(tsk, addr->scope, &addr->addr.nameseq) :
>  		tipc_sk_withdraw(tsk, -addr->scope, &addr->addr.nameseq);
> @@ -706,6 +699,19 @@ static int tipc_bind(struct socket *sock, struct sockaddr *uaddr,
>  	return res;
>  }
>  
> +static int tipc_bind(struct socket *sock, struct sockaddr *skaddr, int alen)
> +{
> +	struct sockaddr_tipc *addr = (struct sockaddr_tipc *)skaddr;
> +
> +	if (alen) {
> +		if (alen < sizeof(struct sockaddr_tipc))
> +			return -EINVAL;
> +		if (addr->addr.nameseq.type < TIPC_RESERVED_TYPES)
> +			return -EACCES;
> +	}
> +	return tipc_sk_bind(sock, skaddr, alen);
> +}
> +
>  /**
>   * tipc_getname - get port ID of socket or peer socket
>   * @sock: socket structure
> diff --git a/net/tipc/socket.h b/net/tipc/socket.h
> index b11575afc66f..02cdf166807d 100644
> --- a/net/tipc/socket.h
> +++ b/net/tipc/socket.h
> @@ -74,7 +74,7 @@ int tipc_dump_done(struct netlink_callback *cb);
>  u32 tipc_sock_get_portid(struct sock *sk);
>  bool tipc_sk_overlimit1(struct sock *sk, struct sk_buff *skb);
>  bool tipc_sk_overlimit2(struct sock *sk, struct sk_buff *skb);
> -
> +int tipc_sk_bind(struct socket *sock, struct sockaddr *skaddr, int alen);
>  int tsk_set_importance(struct sock *sk, int imp);
>  
>  #endif
> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index 5f6f86051c83..cec029349662 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c
> @@ -520,12 +520,12 @@ static int tipc_topsrv_create_listener(struct tipc_topsrv *srv)
>  
>  	saddr.family	                = AF_TIPC;
>  	saddr.addrtype		        = TIPC_ADDR_NAMESEQ;
> -	saddr.addr.nameseq.type	        = TIPC_TOP_SRV;
> +	saddr.addr.nameseq.type         = TIPC_TOP_SRV;
>  	saddr.addr.nameseq.lower	= TIPC_TOP_SRV;
>  	saddr.addr.nameseq.upper	= TIPC_TOP_SRV;
>  	saddr.scope			= TIPC_NODE_SCOPE;
>  
> -	rc = kernel_bind(lsock, (struct sockaddr *)&saddr, sizeof(saddr));
> +	rc = tipc_sk_bind(lsock, (struct sockaddr *)&saddr, sizeof(saddr));
>  	if (rc < 0)
>  		goto err;
>  	rc = kernel_listen(lsock, 0);
> 

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

[Ying X. <yin...@wi...>]

On 10/9/20 1:14 PM, Hoang Huu Le wrote:
> 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...>

Acked-by: Ying Xue <yin...@wi...>

> ---
>  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--;
> 

[tipc-discussion] [net v3 1/1] tipc: fix memory leak caused by tipc_buf_append()

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

Commit ed42989eab57 ("tipc: fix the skb_unshare() in tipc_buf_append()")
replaced skb_unshare() with skb_copy() to not reduce the data reference
counter of the original skb intentionally. This is not the correct
way to handle the cloned skb because it causes memory leak in 2
following cases:
 1/ Sending multicast messages via broadcast link
  The original skb list is cloned to the local skb list for local
  destination. After that, the data reference counter of each skb
  in the original list has the value of 2. This causes each skb not
  to be freed after receiving ACK:
  tipc_link_advance_transmq()
  {
   ...
   /* release skb */
   __skb_unlink(skb, &l->transmq);
   kfree_skb(skb); <-- memory exists after being freed
  }

 2/ Sending multicast messages via replicast link
  Similar to the above case, each skb cannot be freed after purging
  the skb list:
  tipc_mcast_xmit()
  {
   ...
   __skb_queue_purge(pkts); <-- memory exists after being freed
  }

This commit fixes this issue by using skb_unshare() instead. Besides,
to avoid use-after-free error reported by KASAN, the pointer to the
fragment is set to NULL before calling skb_unshare() to make sure that
the original skb is not freed after freeing the fragment 2 times in
case skb_unshare() returns NULL.

Fixes: ed42989eab57 ("tipc: fix the skb_unshare() in tipc_buf_append()")
Acked-by: Jon Maloy <jm...@re...>
Reported-by: Thang Hoang Ngo <tha...@de...>
Signed-off-by: Tung Nguyen <tun...@de...>
---
 net/tipc/msg.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 2a78aa701572..32c79c59052b 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -150,12 +150,11 @@ int tipc_buf_append(struct sk_buff **headbuf, struct sk_buff **buf)
 	if (fragid == FIRST_FRAGMENT) {
 		if (unlikely(head))
 			goto err;
-		if (skb_cloned(frag))
-			frag = skb_copy(frag, GFP_ATOMIC);
+		*buf = NULL;
+		frag = skb_unshare(frag, GFP_ATOMIC);
 		if (unlikely(!frag))
 			goto err;
 		head = *headbuf = frag;
-		*buf = NULL;
 		TIPC_SKB_CB(head)->tail = NULL;
 		if (skb_is_nonlinear(head)) {
 			skb_walk_frags(head, tail) {
-- 
2.17.1

[tipc-discussion] [net v3] 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 reserved types which were really
in use ([0,1]), and block all the rest ([2,63]).

This is risky, 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., most 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 also be made to bypass the checks in tipc_bind()
by introducing a different entry function, tipc_sk_bind().

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 | 24 +++++++++++++++---------
 net/tipc/socket.h |  2 +-
 net/tipc/topsrv.c |  4 ++--
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index e795a8a2955b..222fd53da2d0 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -658,8 +658,8 @@ static int tipc_release(struct socket *sock)
  * NOTE: This routine doesn't need to take the socket lock since it doesn't
  *       access any non-constant socket information.
  */
-static int tipc_bind(struct socket *sock, struct sockaddr *uaddr,
-		     int uaddr_len)
+
+int tipc_sk_bind(struct socket *sock, struct sockaddr *uaddr, int uaddr_len)
 {
 	struct sock *sk = sock->sk;
 	struct sockaddr_tipc *addr = (struct sockaddr_tipc *)uaddr;
@@ -691,13 +691,6 @@ 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)) {
-		res = -EACCES;
-		goto exit;
-	}
-
 	res = (addr->scope >= 0) ?
 		tipc_sk_publish(tsk, addr->scope, &addr->addr.nameseq) :
 		tipc_sk_withdraw(tsk, -addr->scope, &addr->addr.nameseq);
@@ -706,6 +699,19 @@ static int tipc_bind(struct socket *sock, struct sockaddr *uaddr,
 	return res;
 }
 
+static int tipc_bind(struct socket *sock, struct sockaddr *skaddr, int alen)
+{
+	struct sockaddr_tipc *addr = (struct sockaddr_tipc *)skaddr;
+
+	if (alen) {
+		if (alen < sizeof(struct sockaddr_tipc))
+			return -EINVAL;
+		if (addr->addr.nameseq.type < TIPC_RESERVED_TYPES)
+			return -EACCES;
+	}
+	return tipc_sk_bind(sock, skaddr, alen);
+}
+
 /**
  * tipc_getname - get port ID of socket or peer socket
  * @sock: socket structure
diff --git a/net/tipc/socket.h b/net/tipc/socket.h
index b11575afc66f..02cdf166807d 100644
--- a/net/tipc/socket.h
+++ b/net/tipc/socket.h
@@ -74,7 +74,7 @@ int tipc_dump_done(struct netlink_callback *cb);
 u32 tipc_sock_get_portid(struct sock *sk);
 bool tipc_sk_overlimit1(struct sock *sk, struct sk_buff *skb);
 bool tipc_sk_overlimit2(struct sock *sk, struct sk_buff *skb);
-
+int tipc_sk_bind(struct socket *sock, struct sockaddr *skaddr, int alen);
 int tsk_set_importance(struct sock *sk, int imp);
 
 #endif
diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index 5f6f86051c83..cec029349662 100644
--- a/net/tipc/topsrv.c
+++ b/net/tipc/topsrv.c
@@ -520,12 +520,12 @@ static int tipc_topsrv_create_listener(struct tipc_topsrv *srv)
 
 	saddr.family	                = AF_TIPC;
 	saddr.addrtype		        = TIPC_ADDR_NAMESEQ;
-	saddr.addr.nameseq.type	        = TIPC_TOP_SRV;
+	saddr.addr.nameseq.type         = TIPC_TOP_SRV;
 	saddr.addr.nameseq.lower	= TIPC_TOP_SRV;
 	saddr.addr.nameseq.upper	= TIPC_TOP_SRV;
 	saddr.scope			= TIPC_NODE_SCOPE;
 
-	rc = kernel_bind(lsock, (struct sockaddr *)&saddr, sizeof(saddr));
+	rc = tipc_sk_bind(lsock, (struct sockaddr *)&saddr, sizeof(saddr));
 	if (rc < 0)
 		goto err;
 	rc = kernel_listen(lsock, 0);
-- 
2.25.4

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

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

Hi Jon,

I have one idea about how to block the vulnerability described below from happening. Although it's not so elegant, it's quite simple:

As you mentioned below, TIPC_TOP_SRV service is only published in tipc_topsrv_create_listener () from kernel space. So probably we can change the code as below:

tipc_bind()
{
...
	if ((addr->addr.nameseq.type < TIPC_RESERVED_TYPES) {
		res = -EACCES;
		goto exit;
	}
	__tipc_bind();
...
}

Int __tipc_bind()
{
        If (addr->scope >= 0)                
	tipc_sk_publish(tsk, addr->scope, &addr->addr.nameseq) :
         else
                tipc_sk_withdraw(tsk, -addr->scope, &addr->addr.nameseq);
}

tipc_topsrv_create_listener()
{
...
- rc = kernel_bind(lsock, (struct sockaddr *)&saddr, sizeof(saddr));
+ rc = __tipc_bind(lsock, (struct sockaddr *)&saddr, sizeof(saddr));
...
}

Thanks,
Ying

-----Original Message-----
From: jm...@re... <jm...@re...> 
Sent: Sunday, October 25, 2020 3:16 AM
To: tip...@li...
Cc: tun...@de...; hoa...@de...; tuo...@de...; jm...@re...; ma...@do...; xi...@re...; Xue, Ying <Yin...@wi...>; par...@gm...
Subject: [net v2] tipc: add stricter control of reserved service types

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 reserved types which were really in use ([0,1]), and block all the rest ([2,63]).

This is risky, 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 singled out, since it is published from kernel mode and is the very first binding performed when the system is starting.

We now introduce a 'privileged' mode for sockets, marking which of those are entitled to bind to reserved service type values. The only such socket we have so far is the topology server's listener socket, which is identified the way described above. All other bindings to reserved service types are rejected.

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 | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c index e795a8a2955b..a0a144ff84fd 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1,7 +1,8 @@
 /*
  * net/tipc/socket.c: TIPC socket API
  *
- * Copyright (c) 2001-2007, 2012-2017, Ericsson AB
+ * Copyright (c) 2020, Red Hat Inc
+ * Copyright (c) 2001-2007, 2012-2019, Ericsson AB
  * Copyright (c) 2004-2008, 2010-2013, Wind River Systems
  * All rights reserved.
  *
@@ -127,6 +128,8 @@ struct tipc_sock {
 	bool expect_ack;
 	bool nodelay;
 	bool group_is_open;
+	bool privileged;
+	bool kernel;
 };
 
 static int tipc_sk_backlog_rcv(struct sock *sk, struct sk_buff *skb); @@ -479,6 +482,7 @@ static int tipc_sk_create(struct net *net, struct socket *sock,
 	tsk->max_pkt = MAX_PKT_DEFAULT;
 	tsk->maxnagle = 0;
 	tsk->nagle_start = NAGLE_START_INIT;
+	tsk->kernel = !!kern;
 	INIT_LIST_HEAD(&tsk->publications);
 	INIT_LIST_HEAD(&tsk->cong_links);
 	msg = &tsk->phdr;
@@ -665,6 +669,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,13 +696,15 @@ 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_TOP_SRV && tsk->kernel &&
+	    !tipc_nametbl_translate(sock_net(sk), stype, stype, &dnode))
+		tsk->privileged = true;
+
+	if (stype < TIPC_RESERVED_TYPES && !tsk->privileged) {
 		res = -EACCES;
 		goto exit;
 	}
-
 	res = (addr->scope >= 0) ?
 		tipc_sk_publish(tsk, addr->scope, &addr->addr.nameseq) :
 		tipc_sk_withdraw(tsk, -addr->scope, &addr->addr.nameseq);
--
2.25.4

[tipc-discussion] [net v2 1/1] tipc: fix memory leak caused by tipc_buf_append()

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

Commit ed42989eab57 ("fix the skb_unshare() in tipc_buf_append()")
replaced skb_unshare() with skb_copy() to not reduce the data reference
counter of the original skb intentionally. This is not the correct
way to handle the cloned skb because it causes memory leak in 2
following cases:
 1/ Sending multicast messages via broadcast link
  The original skb list is cloned to the local skb list for local
  destination. After that, the data reference counter of each skb
  in the original list has the value of 2. This causes each skb not
  to be freed after receiving ACK:
  tipc_link_advance_transmq()
  {
   ...
   /* release skb */
   __skb_unlink(skb, &l->transmq);
   kfree_skb(skb); <-- memory exists after being freed
  }

 2/ Sending multicast messages via replicast link
  Similar to the above case, each skb cannot be freed after purging
  the skb list:
  tipc_mcast_xmit()
  {
   ...
   __skb_queue_purge(pkts); <-- memory exists after being freed
  }

This commit fixes this issue by using skb_unshare() instead. Besides,
to avoid use-after-free error reported by KASAN, the pointer to the
fragment is set to NULL before calling skb_unshare() to make sure that
the original skb is not freed after freeing the fragment 2 times in
case skb_unshare() returns NULL.

Fixes: ed42989eab57 ("fix the skb_unshare() in tipc_buf_append()")
Acked-by: Jon Maloy <jm...@re...>
Reported-by: Thang Hoang Ngo <tha...@de...>
Signed-off-by: Tung Nguyen <tun...@de...>
---
 net/tipc/msg.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 2a78aa701572..32c79c59052b 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -150,12 +150,11 @@ int tipc_buf_append(struct sk_buff **headbuf, struct sk_buff **buf)
 	if (fragid == FIRST_FRAGMENT) {
 		if (unlikely(head))
 			goto err;
-		if (skb_cloned(frag))
-			frag = skb_copy(frag, GFP_ATOMIC);
+		*buf = NULL;
+		frag = skb_unshare(frag, GFP_ATOMIC);
 		if (unlikely(!frag))
 			goto err;
 		head = *headbuf = frag;
-		*buf = NULL;
 		TIPC_SKB_CB(head)->tail = NULL;
 		if (skb_is_nonlinear(head)) {
 			skb_walk_frags(head, tail) {
-- 
2.17.1

[tipc-discussion] [net v1 1/1] tipc: fix memory leak caused by tipc_buf_append()

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

Commit ed42989eab57 ("fix the skb_unshare() in tipc_buf_append()")
replaced skb_unshare() with skb_copy() to not reduce the data reference
counter of the original skb intentionally. This is not the correct
way to handle the cloned skb because it causes memory leak in 2
following cases:
 1/ Sending multicast messages via broadcast link
  The original skb list is cloned to the local skb list for local
  destination. After that, the data reference counter of each skb
  in the original list has the value of 2. This causes each skb not
  to be freed after receiving ACK:
  tipc_link_advance_transmq()
  {
   ...
   /* release skb */
   __skb_unlink(skb, &l->transmq);
   kfree_skb(skb); <-- memory exists after being freed
  }

 2/ Sending multicast messages via replicast link
  Similar to the above case, each skb cannot be freed after purging
  the skb list:
  tipc_mcast_xmit()
  {
   ...
   __skb_queue_purge(pkts); <-- memory exists after being freed
  }

This commit fixes this issue by using skb_unshare() instead. Besides,
to avoid use-after-free error reported by KASAN, the pointer to the
fragment is set to NULL before calling skb_unshare() to make sure that
the original skb is not freed after freeing the fragment 2 times in
case skb_unshare() returns NULL.

v3: Fix use-after-free error analyzed by Xin

Fixes: ed42989eab57 ("fix the skb_unshare() in tipc_buf_append()")
Reported-by: Thang Hoang Ngo <tha...@de...>
Signed-off-by: Tung Nguyen <tun...@de...>
---
 net/tipc/msg.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 2a78aa701572..32c79c59052b 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -150,12 +150,11 @@ int tipc_buf_append(struct sk_buff **headbuf, struct sk_buff **buf)
 	if (fragid == FIRST_FRAGMENT) {
 		if (unlikely(head))
 			goto err;
-		if (skb_cloned(frag))
-			frag = skb_copy(frag, GFP_ATOMIC);
+		*buf = NULL;
+		frag = skb_unshare(frag, GFP_ATOMIC);
 		if (unlikely(!frag))
 			goto err;
 		head = *headbuf = frag;
-		*buf = NULL;
 		TIPC_SKB_CB(head)->tail = NULL;
 		if (skb_is_nonlinear(head)) {
 			skb_walk_frags(head, tail) {
-- 
2.17.1

Re: [tipc-discussion] [net v1 1/1] tipc: fix memory leak caused by tipc_buf_append()

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

Hi Xin,

Yes, I know that it should not be a problem if skb_free(NULL) is called.
But I relied on your analysis for syzbot report:
"
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.
"

I will check again your new analysis and create the correct patch.

Thanks.
Tung Nguyen

-----Original Message-----
From: Xin Long <luc...@gm...> 
Sent: Monday, October 26, 2020 4:10 PM
To: Tung Quang Nguyen <tun...@de...>
Cc: tip...@li...; Jon Maloy <jm...@re...>; ma...@do...; Ying Xue <yin...@wi...>; Cong Wang <xiy...@gm...>
Subject: Re: [tipc-discussion] [net v1 1/1] tipc: fix memory leak caused by tipc_buf_append()

On Fri, Oct 23, 2020 at 4:20 PM Tung Nguyen
<tun...@de...> wrote:
>
> Commit ed42989eab57 ("fix the skb_unshare() in tipc_buf_append()")
> replaced skb_unshare() with skb_copy() to not reduce the data reference
> counter of the original skb intentionally. This is not the correct
> way to handle the cloned skb because it causes memory leak in 2
> following cases:
>  1/ Sending multicast messages via broadcast link
>   The original skb list is cloned to the local skb list for local
>   destination. After that, the data reference counter of each skb
>   in the original list has the value of 2. This causes each skb not
>   to be freed after receiving ACK:
>   tipc_link_advance_transmq()
>   {
>    ...
>    /* release skb */
>    __skb_unlink(skb, &l->transmq);
>    kfree_skb(skb); <-- memory exists after being freed
>   }
>
>  2/ Sending multicast messages via replicast link
>   Similar to the above case, each skb cannot be freed after purging
>   the skb list:
>   tipc_mcast_xmit()
>   {
>    ...
>    __skb_queue_purge(pkts); <-- memory exists after being freed
>   }
>
> This commit fixes this issue by using skb_unshare() instead. Besides,
> to avoid use-after-free error reported by KASAN, kfree_skb(head) in
> tipc_buf_append() is called only if the pointer to the appending skb
> is not NULL.
>
> v2: improve condition for freeing the appending skb to cover all error
> cases.
>
> Fixes: ed42989eab57 ("fix the skb_unshare() in tipc_buf_append()")
> Reported-by: Thang Hoang Ngo <tha...@de...>
> Signed-off-by: Tung Nguyen <tun...@de...>
> ---
>  net/tipc/msg.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index 2a78aa701572..46c36c5093de 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -150,8 +150,7 @@ int tipc_buf_append(struct sk_buff **headbuf, struct sk_buff **buf)
>         if (fragid == FIRST_FRAGMENT) {
>                 if (unlikely(head))
>                         goto err;
> -               if (skb_cloned(frag))
> -                       frag = skb_copy(frag, GFP_ATOMIC);
> +               frag = skb_unshare(frag, GFP_ATOMIC);
>                 if (unlikely(!frag))
>                         goto err;
>                 head = *headbuf = frag;
> @@ -797,7 +796,8 @@ bool tipc_msg_reassemble(struct sk_buff_head *list, struct sk_buff_head *rcvq)
>         return true;
>  error:
>         pr_warn("Failed do clone local mcast rcv buffer\n");
> -       kfree_skb(head);
> +       if (head)
> +               kfree_skb(head);
Hi Tung,

kfree_skb(NULL) won't cause any use-after-free issue, as kfree_skb(skb)
will return when skb is NULL.

The root cause of use-after-free is as Cong fixed in
commit ed42989eab57 ("fix the skb_unshare() in tipc_buf_append()"):

When skb_unshare() returns NULL, the 'frag' is freed, and on the err
path the 'buf'(==the 'frag') get freed again, then the original skb
is freed.

But that commit indeed caused the memleak on the success path, and
the right fix should be:

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 2a78aa7..73068fb 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -155,6 +155,7 @@ int tipc_buf_append(struct sk_buff **headbuf,
struct sk_buff **buf)
                if (unlikely(!frag))
                        goto err;
                head = *headbuf = frag;
+               kfree_skb(*buf)
                *buf = NULL;
                TIPC_SKB_CB(head)->tail = NULL;
                if (skb_is_nonlinear(head)) {

or:

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 2a78aa7..32c79c5 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -150,12 +150,11 @@ int tipc_buf_append(struct sk_buff **headbuf,
struct sk_buff **buf)
        if (fragid == FIRST_FRAGMENT) {
                if (unlikely(head))
                        goto err;
-               if (skb_cloned(frag))
-                       frag = skb_copy(frag, GFP_ATOMIC);
+               *buf = NULL;
+               frag = skb_unshare(frag, GFP_ATOMIC);
                if (unlikely(!frag))
                        goto err;
                head = *headbuf = frag;
-               *buf = NULL;
                TIPC_SKB_CB(head)->tail = NULL;
                if (skb_is_nonlinear(head)) {
                        skb_walk_frags(head, tail) {

Thanks.

>         return false;
>  }
>
> --
> 2.17.1
>
>
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net v1 1/1] tipc: fix memory leak caused by tipc_buf_append()

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

On Mon, Oct 26, 2020 at 5:30 PM Tung Quang Nguyen
<tun...@de...> wrote:
>
> Hi Xin,
>
> Yes, I know that it should not be a problem if skb_free(NULL) is called.
> But I relied on your analysis for syzbot report:
> "
> 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.
> "
>
> I will check again your new analysis and create the correct patch.
Sorry, I realized it was a false one after double-checking.

>
> Thanks.
> Tung Nguyen
>
> -----Original Message-----
> From: Xin Long <luc...@gm...>
> Sent: Monday, October 26, 2020 4:10 PM
> To: Tung Quang Nguyen <tun...@de...>
> Cc: tip...@li...; Jon Maloy <jm...@re...>; ma...@do...; Ying Xue <yin...@wi...>; Cong Wang <xiy...@gm...>
> Subject: Re: [tipc-discussion] [net v1 1/1] tipc: fix memory leak caused by tipc_buf_append()
>
> On Fri, Oct 23, 2020 at 4:20 PM Tung Nguyen
> <tun...@de...> wrote:
> >
> > Commit ed42989eab57 ("fix the skb_unshare() in tipc_buf_append()")
> > replaced skb_unshare() with skb_copy() to not reduce the data reference
> > counter of the original skb intentionally. This is not the correct
> > way to handle the cloned skb because it causes memory leak in 2
> > following cases:
> >  1/ Sending multicast messages via broadcast link
> >   The original skb list is cloned to the local skb list for local
> >   destination. After that, the data reference counter of each skb
> >   in the original list has the value of 2. This causes each skb not
> >   to be freed after receiving ACK:
> >   tipc_link_advance_transmq()
> >   {
> >    ...
> >    /* release skb */
> >    __skb_unlink(skb, &l->transmq);
> >    kfree_skb(skb); <-- memory exists after being freed
> >   }
> >
> >  2/ Sending multicast messages via replicast link
> >   Similar to the above case, each skb cannot be freed after purging
> >   the skb list:
> >   tipc_mcast_xmit()
> >   {
> >    ...
> >    __skb_queue_purge(pkts); <-- memory exists after being freed
> >   }
> >
> > This commit fixes this issue by using skb_unshare() instead. Besides,
> > to avoid use-after-free error reported by KASAN, kfree_skb(head) in
> > tipc_buf_append() is called only if the pointer to the appending skb
> > is not NULL.
> >
> > v2: improve condition for freeing the appending skb to cover all error
> > cases.
> >
> > Fixes: ed42989eab57 ("fix the skb_unshare() in tipc_buf_append()")
> > Reported-by: Thang Hoang Ngo <tha...@de...>
> > Signed-off-by: Tung Nguyen <tun...@de...>
> > ---
> >  net/tipc/msg.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> > index 2a78aa701572..46c36c5093de 100644
> > --- a/net/tipc/msg.c
> > +++ b/net/tipc/msg.c
> > @@ -150,8 +150,7 @@ int tipc_buf_append(struct sk_buff **headbuf, struct sk_buff **buf)
> >         if (fragid == FIRST_FRAGMENT) {
> >                 if (unlikely(head))
> >                         goto err;
> > -               if (skb_cloned(frag))
> > -                       frag = skb_copy(frag, GFP_ATOMIC);
> > +               frag = skb_unshare(frag, GFP_ATOMIC);
> >                 if (unlikely(!frag))
> >                         goto err;
> >                 head = *headbuf = frag;
> > @@ -797,7 +796,8 @@ bool tipc_msg_reassemble(struct sk_buff_head *list, struct sk_buff_head *rcvq)
> >         return true;
> >  error:
> >         pr_warn("Failed do clone local mcast rcv buffer\n");
> > -       kfree_skb(head);
> > +       if (head)
> > +               kfree_skb(head);
> Hi Tung,
>
> kfree_skb(NULL) won't cause any use-after-free issue, as kfree_skb(skb)
> will return when skb is NULL.
>
> The root cause of use-after-free is as Cong fixed in
> commit ed42989eab57 ("fix the skb_unshare() in tipc_buf_append()"):
>
> When skb_unshare() returns NULL, the 'frag' is freed, and on the err
> path the 'buf'(==the 'frag') get freed again, then the original skb
> is freed.
>
> But that commit indeed caused the memleak on the success path, and
> the right fix should be:
>
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index 2a78aa7..73068fb 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -155,6 +155,7 @@ int tipc_buf_append(struct sk_buff **headbuf,
> struct sk_buff **buf)
>                 if (unlikely(!frag))
>                         goto err;
>                 head = *headbuf = frag;
> +               kfree_skb(*buf)
>                 *buf = NULL;
>                 TIPC_SKB_CB(head)->tail = NULL;
>                 if (skb_is_nonlinear(head)) {
This should be:

@@ -150,10 +150,12 @@ int tipc_buf_append(struct sk_buff **headbuf,
struct sk_buff **buf)
        if (fragid == FIRST_FRAGMENT) {
                if (unlikely(head))
                        goto err;
-               if (skb_cloned(frag))
+               if (skb_cloned(frag)) {
                        frag = skb_copy(frag, GFP_ATOMIC);
-               if (unlikely(!frag))
-                       goto err;
+                       if (unlikely(!frag))
+                               goto err;
+                       kfree_skb(*buf)
+               }

>
> or:
>
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index 2a78aa7..32c79c5 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -150,12 +150,11 @@ int tipc_buf_append(struct sk_buff **headbuf,
> struct sk_buff **buf)
>         if (fragid == FIRST_FRAGMENT) {
>                 if (unlikely(head))
>                         goto err;
> -               if (skb_cloned(frag))
> -                       frag = skb_copy(frag, GFP_ATOMIC);
> +               *buf = NULL;
> +               frag = skb_unshare(frag, GFP_ATOMIC);
>                 if (unlikely(!frag))
>                         goto err;
>                 head = *headbuf = frag;
> -               *buf = NULL;
>                 TIPC_SKB_CB(head)->tail = NULL;
>                 if (skb_is_nonlinear(head)) {
>                         skb_walk_frags(head, tail) {
>
> Thanks.
>
> >         return false;
> >  }
> >
> > --
> > 2.17.1
> >
> >
> >
> > _______________________________________________
> > tipc-discussion mailing list
> > tip...@li...
> > https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net v1 1/1] tipc: fix memory leak caused by tipc_buf_append()

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

On Fri, Oct 23, 2020 at 4:20 PM Tung Nguyen
<tun...@de...> wrote:
>
> Commit ed42989eab57 ("fix the skb_unshare() in tipc_buf_append()")
> replaced skb_unshare() with skb_copy() to not reduce the data reference
> counter of the original skb intentionally. This is not the correct
> way to handle the cloned skb because it causes memory leak in 2
> following cases:
>  1/ Sending multicast messages via broadcast link
>   The original skb list is cloned to the local skb list for local
>   destination. After that, the data reference counter of each skb
>   in the original list has the value of 2. This causes each skb not
>   to be freed after receiving ACK:
>   tipc_link_advance_transmq()
>   {
>    ...
>    /* release skb */
>    __skb_unlink(skb, &l->transmq);
>    kfree_skb(skb); <-- memory exists after being freed
>   }
>
>  2/ Sending multicast messages via replicast link
>   Similar to the above case, each skb cannot be freed after purging
>   the skb list:
>   tipc_mcast_xmit()
>   {
>    ...
>    __skb_queue_purge(pkts); <-- memory exists after being freed
>   }
>
> This commit fixes this issue by using skb_unshare() instead. Besides,
> to avoid use-after-free error reported by KASAN, kfree_skb(head) in
> tipc_buf_append() is called only if the pointer to the appending skb
> is not NULL.
>
> v2: improve condition for freeing the appending skb to cover all error
> cases.
>
> Fixes: ed42989eab57 ("fix the skb_unshare() in tipc_buf_append()")
> Reported-by: Thang Hoang Ngo <tha...@de...>
> Signed-off-by: Tung Nguyen <tun...@de...>
> ---
>  net/tipc/msg.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index 2a78aa701572..46c36c5093de 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -150,8 +150,7 @@ int tipc_buf_append(struct sk_buff **headbuf, struct sk_buff **buf)
>         if (fragid == FIRST_FRAGMENT) {
>                 if (unlikely(head))
>                         goto err;
> -               if (skb_cloned(frag))
> -                       frag = skb_copy(frag, GFP_ATOMIC);
> +               frag = skb_unshare(frag, GFP_ATOMIC);
>                 if (unlikely(!frag))
>                         goto err;
>                 head = *headbuf = frag;
> @@ -797,7 +796,8 @@ bool tipc_msg_reassemble(struct sk_buff_head *list, struct sk_buff_head *rcvq)
>         return true;
>  error:
>         pr_warn("Failed do clone local mcast rcv buffer\n");
> -       kfree_skb(head);
> +       if (head)
> +               kfree_skb(head);
Hi Tung,

kfree_skb(NULL) won't cause any use-after-free issue, as kfree_skb(skb)
will return when skb is NULL.

The root cause of use-after-free is as Cong fixed in
commit ed42989eab57 ("fix the skb_unshare() in tipc_buf_append()"):

When skb_unshare() returns NULL, the 'frag' is freed, and on the err
path the 'buf'(==the 'frag') get freed again, then the original skb
is freed.

But that commit indeed caused the memleak on the success path, and
the right fix should be:

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 2a78aa7..73068fb 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -155,6 +155,7 @@ int tipc_buf_append(struct sk_buff **headbuf,
struct sk_buff **buf)
                if (unlikely(!frag))
                        goto err;
                head = *headbuf = frag;
+               kfree_skb(*buf)
                *buf = NULL;
                TIPC_SKB_CB(head)->tail = NULL;
                if (skb_is_nonlinear(head)) {

or:

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 2a78aa7..32c79c5 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -150,12 +150,11 @@ int tipc_buf_append(struct sk_buff **headbuf,
struct sk_buff **buf)
        if (fragid == FIRST_FRAGMENT) {
                if (unlikely(head))
                        goto err;
-               if (skb_cloned(frag))
-                       frag = skb_copy(frag, GFP_ATOMIC);
+               *buf = NULL;
+               frag = skb_unshare(frag, GFP_ATOMIC);
                if (unlikely(!frag))
                        goto err;
                head = *headbuf = frag;
-               *buf = NULL;
                TIPC_SKB_CB(head)->tail = NULL;
                if (skb_is_nonlinear(head)) {
                        skb_walk_frags(head, tail) {

Thanks.

>         return false;
>  }
>
> --
> 2.17.1
>
>
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [net v1 1/1] tipc: fix memory leak caused by tipc_buf_append()

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

Commit ed42989eab57 ("fix the skb_unshare() in tipc_buf_append()")
replaced skb_unshare() with skb_copy() to not reduce the data reference
counter of the original skb intentionally. This is not the correct
way to handle the cloned skb because it causes memory leak in 2
following cases:
 1/ Sending multicast messages via broadcast link
  The original skb list is cloned to the local skb list for local
  destination. After that, the data reference counter of each skb
  in the original list has the value of 2. This causes each skb not
  to be freed after receiving ACK:
  tipc_link_advance_transmq()
  {
   ...
   /* release skb */
   __skb_unlink(skb, &l->transmq);
   kfree_skb(skb); <-- memory exists after being freed
  }

 2/ Sending multicast messages via replicast link
  Similar to the above case, each skb cannot be freed after purging
  the skb list:
  tipc_mcast_xmit()
  {
   ...
   __skb_queue_purge(pkts); <-- memory exists after being freed
  }

This commit fixes this issue by using skb_unshare() instead. Besides,
to avoid use-after-free error reported by KASAN, kfree_skb(head) in
tipc_buf_reassemble() is called only if the pointer to the appending skb
is not NULL.

Fixes: ed42989eab57 ("fix the skb_unshare() in tipc_buf_append()")
Acked-by: Jon Maloy <jm...@re...>
Reported-by: Thang Hoang Ngo <tha...@de...>
Signed-off-by: Tung Nguyen <tun...@de...>
---
 net/tipc/msg.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 2a78aa701572..46c36c5093de 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -150,8 +150,7 @@ int tipc_buf_append(struct sk_buff **headbuf, struct sk_buff **buf)
 	if (fragid == FIRST_FRAGMENT) {
 		if (unlikely(head))
 			goto err;
-		if (skb_cloned(frag))
-			frag = skb_copy(frag, GFP_ATOMIC);
+		frag = skb_unshare(frag, GFP_ATOMIC);
 		if (unlikely(!frag))
 			goto err;
 		head = *headbuf = frag;
@@ -797,7 +796,8 @@ bool tipc_msg_reassemble(struct sk_buff_head *list, struct sk_buff_head *rcvq)
 	return true;
 error:
 	pr_warn("Failed do clone local mcast rcv buffer\n");
-	kfree_skb(head);
+	if (head)
+		kfree_skb(head);
 	return false;
 }
 
-- 
2.17.1

[tipc-discussion] [net v2] 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 reserved types which were really
in use ([0,1]), and block all the rest ([2,63]).

This is risky, 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 singled out, since it is published from
kernel mode and is the very first binding performed when the system
is starting.

We now introduce a 'privileged' mode for sockets, marking which of
those are entitled to bind to reserved service type values. The only
such socket we have so far is the topology server's listener socket,
which is identified the way described above. All other bindings to
reserved service types are rejected.

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 | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index e795a8a2955b..a0a144ff84fd 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1,7 +1,8 @@
 /*
  * net/tipc/socket.c: TIPC socket API
  *
- * Copyright (c) 2001-2007, 2012-2017, Ericsson AB
+ * Copyright (c) 2020, Red Hat Inc
+ * Copyright (c) 2001-2007, 2012-2019, Ericsson AB
  * Copyright (c) 2004-2008, 2010-2013, Wind River Systems
  * All rights reserved.
  *
@@ -127,6 +128,8 @@ struct tipc_sock {
 	bool expect_ack;
 	bool nodelay;
 	bool group_is_open;
+	bool privileged;
+	bool kernel;
 };
 
 static int tipc_sk_backlog_rcv(struct sock *sk, struct sk_buff *skb);
@@ -479,6 +482,7 @@ static int tipc_sk_create(struct net *net, struct socket *sock,
 	tsk->max_pkt = MAX_PKT_DEFAULT;
 	tsk->maxnagle = 0;
 	tsk->nagle_start = NAGLE_START_INIT;
+	tsk->kernel = !!kern;
 	INIT_LIST_HEAD(&tsk->publications);
 	INIT_LIST_HEAD(&tsk->cong_links);
 	msg = &tsk->phdr;
@@ -665,6 +669,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,13 +696,15 @@ 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_TOP_SRV && tsk->kernel &&
+	    !tipc_nametbl_translate(sock_net(sk), stype, stype, &dnode))
+		tsk->privileged = true;
+
+	if (stype < TIPC_RESERVED_TYPES && !tsk->privileged) {
 		res = -EACCES;
 		goto exit;
 	}
-
 	res = (addr->scope >= 0) ?
 		tipc_sk_publish(tsk, addr->scope, &addr->addr.nameseq) :
 		tipc_sk_withdraw(tsk, -addr->scope, &addr->addr.nameseq);
-- 
2.25.4

Re: [tipc-discussion] [net v1 1/1] tipc: fix memory leak caused by tipc_buf_append()

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

On 10/23/20 4:19 AM, Tung Nguyen wrote:
> Commit ed42989eab57 ("fix the skb_unshare() in tipc_buf_append()")
> replaced skb_unshare() with skb_copy() to not reduce the data reference
> counter of the original skb intentionally. This is not the correct
> way to handle the cloned skb because it causes memory leak in 2
> following cases:
>   1/ Sending multicast messages via broadcast link
>    The original skb list is cloned to the local skb list for local
>    destination. After that, the data reference counter of each skb
>    in the original list has the value of 2. This causes each skb not
>    to be freed after receiving ACK:
>    tipc_link_advance_transmq()
>    {
>     ...
>     /* release skb */
>     __skb_unlink(skb, &l->transmq);
>     kfree_skb(skb); <-- memory exists after being freed
>    }
>
>   2/ Sending multicast messages via replicast link
>    Similar to the above case, each skb cannot be freed after purging
>    the skb list:
>    tipc_mcast_xmit()
>    {
>     ...
>     __skb_queue_purge(pkts); <-- memory exists after being freed
>    }
>
> This commit fixes this issue by using skb_unshare() instead. Besides,
> to avoid use-after-free error reported by KASAN, kfree_skb(head) in
> tipc_buf_append() is called only if the pointer to the appending skb
> is not NULL.
>
> v2: improve condition for freeing the appending skb to cover all error
> cases.
>
> Fixes: ed42989eab57 ("fix the skb_unshare() in tipc_buf_append()")
> Reported-by: Thang Hoang Ngo <tha...@de...>
> Signed-off-by: Tung Nguyen <tun...@de...>
> ---
>   net/tipc/msg.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index 2a78aa701572..46c36c5093de 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -150,8 +150,7 @@ int tipc_buf_append(struct sk_buff **headbuf, struct sk_buff **buf)
>   	if (fragid == FIRST_FRAGMENT) {
>   		if (unlikely(head))
>   			goto err;
> -		if (skb_cloned(frag))
> -			frag = skb_copy(frag, GFP_ATOMIC);
> +		frag = skb_unshare(frag, GFP_ATOMIC);
>   		if (unlikely(!frag))
>   			goto err;
>   		head = *headbuf = frag;
> @@ -797,7 +796,8 @@ bool tipc_msg_reassemble(struct sk_buff_head *list, struct sk_buff_head *rcvq)
>   	return true;
>   error:
>   	pr_warn("Failed do clone local mcast rcv buffer\n");
> -	kfree_skb(head);
> +	if (head)
> +		kfree_skb(head);
>   	return false;
>   }
>   
Acked-by: Jon Maloy <jm...@re...>