Re: [tipc-discussion] [net-next 00/16] tipc: cleanups and simplifications

[Ying Xue]

Thank you for your patience Jon!

This series looks good to me.

Acked-by: Ying Xue 

On 12/9/20 2:49 AM, jma...@redhat.com wrote:
> From: Jon Maloy 
> 
> We make a number of simplifications and cleanups, especially to call 
> signatures
> in the binding table. This makes the code easier to understand and serves as a
> preparation for an upcoming functional addition.
> 
> Jon Maloy (16):
>   tipc: re-organize members of struct publication
>   tipc: move creation of publication item one level up in call chain
>   tipc: introduce new unified address type for internal use
>   tipc: simplify signature of tipc_namtbl_publish()
>   tipc: simplify call signatures for publication creation
>   tipc: simplify signature of tipc_nametbl_withdraw() functions
>   tipc: rename binding table lookup functions
>   tipc: refactor tipc_sendmsg() and tipc_lookup_anycast()
>   tipc: simplify signature of tipc_namtbl_lookup_mcast_sockets()
>   tipc: simplify signature of tipc_nametbl_lookup_mcast_nodes()
>   tipc: simplify signature of tipc_nametbl_lookup_group()
>   tipc: simplify signature of tipc_service_find_range()
>   tipc: simplify signature of tipc_find_service()
>   tipc: simplify api between binding table and topology server
>   tipc: add host-endian copy of user subscription to struct
> tipc_subscription
>   tipc: remove some unnecessary warnings
> 
>  net/tipc/addr.h   |  44 +
>  net/tipc/msg.c|  23 ++-
>  net/tipc/name_distr.c |  89 +
>  net/tipc/name_table.c | 419 ++
>  net/tipc/name_table.h |  64 ---
>  net/tipc/net.c|   8 +-
>  net/tipc/node.c   |  28 +--
>  net/tipc/socket.c | 313 +++
>  net/tipc/subscr.c |  84 +
>  net/tipc/subscr.h |  12 +-
>  10 files changed, 567 insertions(+), 517 deletions(-)
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH 01/10 net-next] net/tipc: fix tipc header files for kernel-doc

[Ying Xue]

Please see my comments about parameters marked with "FIXME":

On 11/25/20 12:20 PM, Randy Dunlap wrote:
> Fix tipc header files for adding to the networking docbook.
> 
> Remove some uses of "/**" that were not kernel-doc notation.
> 
> Fix some source formatting to eliminate Sphinx warnings.
> 
> Add missing struct member and function argument kernel-doc descriptions.
> 
> Documentation/networking/tipc:18: ../net/tipc/name_table.h:65: WARNING: 
> Unexpected indentation.
> Documentation/networking/tipc:18: ../net/tipc/name_table.h:66: WARNING: Block 
> quote ends without a blank line; unexpected unindent.
> 
> ../net/tipc/bearer.h:128: warning: Function parameter or member 'min_win' not 
> described in 'tipc_media'
> ../net/tipc/bearer.h:128: warning: Function parameter or member 'max_win' not 
> described in 'tipc_media'
> 
> ../net/tipc/bearer.h:171: warning: Function parameter or member 'min_win' not 
> described in 'tipc_bearer'
> ../net/tipc/bearer.h:171: warning: Function parameter or member 'max_win' not 
> described in 'tipc_bearer'
> ../net/tipc/bearer.h:171: warning: Function parameter or member 'disc' not 
> described in 'tipc_bearer'
> ../net/tipc/bearer.h:171: warning: Function parameter or member 'up' not 
> described in 'tipc_bearer'
> ../net/tipc/bearer.h:171: warning: Function parameter or member 'refcnt' not 
> described in 'tipc_bearer'
> 
> ../net/tipc/name_distr.h:68: warning: Function parameter or member 'port' not 
> described in 'distr_item'
> 
> ../net/tipc/name_table.h:111: warning: Function parameter or member 
> 'services' not described in 'name_table'
> ../net/tipc/name_table.h:111: warning: Function parameter or member 
> 'cluster_scope_lock' not described in 'name_table'
> ../net/tipc/name_table.h:111: warning: Function parameter or member 
> 'rc_dests' not described in 'name_table'
> ../net/tipc/name_table.h:111: warning: Function parameter or member 'snd_nxt' 
> not described in 'name_table'
> 
> ../net/tipc/subscr.h:67: warning: Function parameter or member 'kref' not 
> described in 'tipc_subscription'
> ../net/tipc/subscr.h:67: warning: Function parameter or member 'net' not 
> described in 'tipc_subscription'
> ../net/tipc/subscr.h:67: warning: Function parameter or member 'service_list' 
> not described in 'tipc_subscription'
> ../net/tipc/subscr.h:67: warning: Function parameter or member 'conid' not 
> described in 'tipc_subscription'
> ../net/tipc/subscr.h:67: warning: Function parameter or member 'inactive' not 
> described in 'tipc_subscription'
> ../net/tipc/subscr.h:67: warning: Function parameter or member 'lock' not 
> described in 'tipc_subscription'
> 
> Signed-off-by: Randy Dunlap 
> Cc: Jon Maloy 
> Cc: Ying Xue 
> Cc: net...@vger.kernel.org
> Cc: tipc-discussion@lists.sourceforge.net
> Cc: "David S. Miller" 
> Cc: Jakub Kicinski 
> ---
>  net/tipc/bearer.h |   10 +++---
>  net/tipc/crypto.h |6 +++---
>  net/tipc/name_distr.h |2 +-
>  net/tipc/name_table.h |9 ++---
>  net/tipc/subscr.h |   11 +++
>  5 files changed, 24 insertions(+), 14 deletions(-)
> 
> --- linux-next-20201102.orig/net/tipc/bearer.h
> +++ linux-next-20201102/net/tipc/bearer.h
> @@ -93,7 +93,8 @@ struct tipc_bearer;
>   * @raw2addr: convert from raw addr format to media addr format
>   * @priority: default link (and bearer) priority
>   * @tolerance: default time (in ms) before declaring link failure
> - * @window: default window (in packets) before declaring link congestion
> + * @min_win: minimum window (in packets) before declaring link congestion
> + * @max_win: maximum window (in packets) before declaring link congestion
>   * @mtu: max packet size bearer can support for media type not dependent on
>   * underlying device MTU
>   * @type_id: TIPC media identifier
> @@ -138,12 +139,15 @@ struct tipc_media {
>   * @pt: packet type for bearer
>   * @rcu: rcu struct for tipc_bearer
>   * @priority: default link priority for bearer
> - * @window: default window size for bearer
> + * @min_win: minimum window (in packets) before declaring link congestion
> + * @max_win: maximum window (in packets) before declaring link congestion
>   * @tolerance: default link tolerance for bearer
>   * @domain: network domain to which links can be established
>   * @identity: array index of this bearer within TIPC bearer array
> - * @link_req: ptr to (optional) structure making periodic link setup requests
> + * @disc: ptr to link setup request
>   * @net_plane: network plane ('A' through 'H') currently associated with 
> bearer
> + * @up: bearer up flag (bit 0)
> + * @refcnt: tipc_bearer reference counter
>   *
>   * Note: media-specific code 

Re: [tipc-discussion] [PATCH 00/10 net-next] net/tipc: fix all kernel-doc and add TIPC networking chapter

[Ying Xue]

On 11/25/20 12:20 PM, Randy Dunlap wrote:
> 
> Question: is net/tipc/discover.c, in tipc_disc_delete() kernel-doc,
> what is the word "duest"?  Should it be changed?

The "duest" is a typo, and it should be "dest" defined as below:
struct tipc_discoverer {
u32 bearer_id;
struct tipc_media_addr dest; ===> "dest"
struct net *net;
u32 domain;
int num_nodes;
spinlock_t lock;
struct sk_buff *skb;
struct timer_list timer;
unsigned long timer_intv;
};


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

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

[Ying Xue]

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

Acked-by: Ying Xue 

> 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(-)
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

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

[Ying Xue]

On 11/12/20 5:34 PM, Kang Wenlin wrote:
> From: Wenlin Kang 
> 
> 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 

Acked-by: Ying Xue 

> ---
>  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 */
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

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

[Ying Xue]

On 11/12/20 9:26 AM, jma...@redhat.com 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 mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

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

[Ying Xue]

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 

Acked-by: Ying Xue 

> ---
>  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(>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(>nametbl_lock);
> - list_for_each_entry_safe(e, tmp, >dist_queue, next) {
> - if (e->node != addr)
> - continue;
> - list_del(>next);
> - kfree(e);
> - }
> - spin_unlock_bh(>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(>nametbl_lock);
>   if (!(capabilities & TIPC_NAMED_BCAST))
>   nt->rc_dests--;
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

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

[Ying Xue]

On 10/10/20 10:56 PM, jma...@redhat.com wrote:
> From: Jon Maloy 
> 
> TIPC reserves 64 service types for current and future internal use.
> Therefore, the bind() function is meant to block regular user sockets
> from being bound to these values, while it should let through such
> bindings from internal users.
> 
> However, since we at the design moment saw no way to distinguish
> between regular and internal users the filter function ended up
> with allowing all bindings of the types which were really in use
> ([0,1]), and block all the rest ([2,63]).
> 
> This is dangerous, since a regular user may bind to the service type
> representing the topology server (TIPC_TOP_SRV == 1) or the one used
> for indicating neigboring node status (TIPC_CFG_SRV == 0), and wreak
> havoc for users of those services. I.e., practically all users.
> 
> The reality is however that TIPC_CFG_SRV never is bound through the
> bind() function, since it doesn't represent a regular socket, and
> TIPC_TOP_SRV can easily be filtered out, since it is the very first
> binding performed when the system is starting.
> 
> We can hence block TIPC_CFG_SRV completely, and only allow TIPC_TOP_SRV
> to be bound once, and the correct behavior is achieved. This is what we
> do in this commit.
> 
> It should be noted that, although this is a change of the API semantics,
> there is no risk we will break any currently working applications by
> doing this. Any application trying to bind to the values in question
> would be badly broken from the outset, so there is no chance we would
> find any such applications in real-world production systems.
> 
> Signed-off-by: Jon Maloy 

Acked-by: Ying Xue 

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


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

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

[Ying Xue]

On 10/21/20 10:58 PM, Jon Maloy wrote:
> That is extremely simple. Take the hello_server under 
> tipcutils/demos/hello_world and change the server name to {1,1}.
> It works! And it shouldn't of course, because it will steal traffic 
> directed to the toplogy server.

Thanks Jon for your explanation!


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

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

[Ying Xue]

On 10/10/20 10:56 PM, jma...@redhat.com wrote:
> From: Jon Maloy 
> 
> TIPC reserves 64 service types for current and future internal use.
> Therefore, the bind() function is meant to block regular user sockets
> from being bound to these values, while it should let through such
> bindings from internal users.
> 
> However, since we at the design moment saw no way to distinguish
> between regular and internal users the filter function ended up
> with allowing all bindings of the types which were really in use
> ([0,1]), and block all the rest ([2,63]).
> 
> This is dangerous, since a regular user may bind to the service type
> representing the topology server (TIPC_TOP_SRV == 1) or the one used
> for indicating neigboring node status (TIPC_CFG_SRV == 0), and wreak
> havoc for users of those services. I.e., practically all users.
> 

Sorry, Jon. I could not understand how such scenarios described above
happened. Can you please take an example to demonstrate a regular user
can bind to the same service type as TIPC_TOP_SRV or TIPC_CFG_SRV under
the current code logic?

Thanks,
Ying

> The reality is however that TIPC_CFG_SRV never is bound through the
> bind() function, since it doesn't represent a regular socket, and
> TIPC_TOP_SRV can easily be filtered out, since it is the very first
> binding performed when the system is starting.
> 
> We can hence block TIPC_CFG_SRV completely, and only allow TIPC_TOP_SRV
> to be bound once, and the correct behavior is achieved. This is what we
> do in this commit.
> 
> It should be noted that, although this is a change of the API semantics,
> there is no risk we will break any currently working applications by
> doing this. Any application trying to bind to the values in question
> would be badly broken from the outset, so there is no chance we would
> find any such applications in real-world production systems.
> 
> Signed-off-by: Jon Maloy 
> ---
>  net/tipc/socket.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index e795a8a2955b..67875a5761d0 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -665,6 +665,7 @@ static int tipc_bind(struct socket *sock, struct sockaddr 
> *uaddr,
>   struct sockaddr_tipc *addr = (struct sockaddr_tipc *)uaddr;
>   struct tipc_sock *tsk = tipc_sk(sk);
>   int res = -EINVAL;
> + u32 stype, dnode;
>  
>   lock_sock(sk);
>   if (unlikely(!uaddr_len)) {
> @@ -691,9 +692,10 @@ static int tipc_bind(struct socket *sock, struct 
> sockaddr *uaddr,
>   goto exit;
>   }
>  
> - if ((addr->addr.nameseq.type < TIPC_RESERVED_TYPES) &&
> - (addr->addr.nameseq.type != TIPC_TOP_SRV) &&
> - (addr->addr.nameseq.type != TIPC_CFG_SRV)) {
> + stype = addr->addr.nameseq.type;
> + if (stype < TIPC_RESERVED_TYPES &&
> + (stype != TIPC_TOP_SRV ||
> +  tipc_nametbl_translate(sock_net(sk), stype, stype, ))) {
>   res = -EACCES;
>   goto exit;
>   }
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH net 2/2] tipc: set ub->ifindex for local ipv6 address

[Ying Xue]

On 8/3/20 11:34 PM, Xin Long wrote:
> Without ub->ifindex set for ipv6 address in tipc_udp_enable(),
> ipv6_sock_mc_join() may make the wrong dev join the multicast
> address in enable_mcast(). This causes that tipc links would
> never be created.
> 
> So fix it by getting the right netdev and setting ub->ifindex,
> as it does for ipv4 address.
> 
> Reported-by: Shuang Li 
> Signed-off-by: Xin Long 

Acked-by: Ying Xue 

> ---
>  net/tipc/udp_media.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
> index 28a283f..9dec596 100644
> --- a/net/tipc/udp_media.c
> +++ b/net/tipc/udp_media.c
> @@ -738,6 +738,13 @@ static int tipc_udp_enable(struct net *net, struct 
> tipc_bearer *b,
>   b->mtu = b->media->mtu;
>  #if IS_ENABLED(CONFIG_IPV6)
>   } else if (local.proto == htons(ETH_P_IPV6)) {
> + struct net_device *dev;
> +
> + dev = ipv6_dev_find(net, );
> + if (!dev) {
> + err = -ENODEV;
> + goto err;
> + }
>   udp_conf.family = AF_INET6;
>   udp_conf.use_udp6_tx_checksums = true;
>   udp_conf.use_udp6_rx_checksums = true;
> @@ -745,6 +752,7 @@ static int tipc_udp_enable(struct net *net, struct 
> tipc_bearer *b,
>   udp_conf.local_ip6 = in6addr_any;
>   else
>   udp_conf.local_ip6 = local.ipv6;
> + ub->ifindex = dev->ifindex;
>   b->mtu = 1280;
>  #endif
>   } else {
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH net 1/2] ipv6: add ipv6_dev_find()

[Ying Xue]

On 8/3/20 11:34 PM, Xin Long wrote:
> This is to add an ip_dev_find like function for ipv6, used to find
> the dev by saddr.
> 
> It will be used by TIPC protocol. So also export it.
> 
> Signed-off-by: Xin Long 

Acked-by: Ying Xue 

> ---
>  include/net/addrconf.h |  2 ++
>  net/ipv6/addrconf.c| 39 +++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/include/net/addrconf.h b/include/net/addrconf.h
> index 8418b7d..ba3f6c15 100644
> --- a/include/net/addrconf.h
> +++ b/include/net/addrconf.h
> @@ -97,6 +97,8 @@ bool ipv6_chk_custom_prefix(const struct in6_addr *addr,
>  
>  int ipv6_chk_prefix(const struct in6_addr *addr, struct net_device *dev);
>  
> +struct net_device *ipv6_dev_find(struct net *net, const struct in6_addr 
> *addr);
> +
>  struct inet6_ifaddr *ipv6_get_ifaddr(struct net *net,
>const struct in6_addr *addr,
>struct net_device *dev, int strict);
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 840bfdb..857d6f9 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -1983,6 +1983,45 @@ int ipv6_chk_prefix(const struct in6_addr *addr, 
> struct net_device *dev)
>  }
>  EXPORT_SYMBOL(ipv6_chk_prefix);
>  
> +/**
> + * ipv6_dev_find - find the first device with a given source address.
> + * @net: the net namespace
> + * @addr: the source address
> + *
> + * The caller should be protected by RCU, or RTNL.
> + */
> +struct net_device *ipv6_dev_find(struct net *net, const struct in6_addr 
> *addr)
> +{
> + unsigned int hash = inet6_addr_hash(net, addr);
> + struct inet6_ifaddr *ifp, *result = NULL;
> + struct net_device *dev = NULL;
> +
> + rcu_read_lock();
> + hlist_for_each_entry_rcu(ifp, _addr_lst[hash], addr_lst) {
> + if (net_eq(dev_net(ifp->idev->dev), net) &&
> + ipv6_addr_equal(>addr, addr)) {
> + result = ifp;
> + break;
> + }
> + }
> +
> + if (!result) {
> + struct rt6_info *rt;
> +
> + rt = rt6_lookup(net, addr, NULL, 0, NULL, 0);
> + if (rt) {
> + dev = rt->dst.dev;
> + ip6_rt_put(rt);
> + }
> + } else {
> + dev = result->idev->dev;
> + }
> + rcu_read_unlock();
> +
> + return dev;
> +}
> +EXPORT_SYMBOL(ipv6_dev_find);
> +
>  struct inet6_ifaddr *ipv6_get_ifaddr(struct net *net, const struct in6_addr 
> *addr,
>struct net_device *dev, int strict)
>  {
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH net-next] tipc: Use is_broadcast_ether_addr() instead of memcmp()

[Ying Xue]

On 8/3/20 10:00 AM, Huang Guobin wrote:
> Using is_broadcast_ether_addr() instead of directly use
> memcmp() to determine if the ethernet address is broadcast
> address.
> 
> spatch with a semantic match is used to found this problem.
> (http://coccinelle.lip6.fr/)
> 
> Signed-off-by: Huang Guobin 

Acked-by: Ying Xue 

> ---
>  net/tipc/eth_media.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c
> index 8b0bb600602d..c68019697cfe 100644
> --- a/net/tipc/eth_media.c
> +++ b/net/tipc/eth_media.c
> @@ -62,12 +62,10 @@ static int tipc_eth_raw2addr(struct tipc_bearer *b,
>struct tipc_media_addr *addr,
>char *msg)
>  {
> - char bcast_mac[ETH_ALEN] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
> -
>   memset(addr, 0, sizeof(*addr));
>   ether_addr_copy(addr->value, msg);
>   addr->media_id = TIPC_MEDIA_TYPE_ETH;
> - addr->broadcast = !memcmp(addr->value, bcast_mac, ETH_ALEN);
> + addr->broadcast = is_broadcast_ether_addr(addr->value);
>   return 0;
>  }
>  
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net] tipc: fix failed service subscription deletion

[Ying Xue]

On 5/11/20 11:59 AM, Tuong Lien wrote:
> When a service subscription is expired or canceled by user, it needs to
> be deleted from the subscription list, so that new subscriptions can be
> registered (max = 65535 per net). However, there are two issues in code
> that can cause such an unused subscription to persist:
> 
> 1) The 'tipc_conn_delete_sub()' has a loop on the subscription list but
> it makes a break shortly when the 1st subscription differs from the one
> specified, so the subscription will not be deleted.
> 
> 2) In case a subscription is canceled, the code to remove the
> 'TIPC_SUB_CANCEL' flag from the subscription filter does not work if it
> is a local subscription (i.e. the little endian isn't involved). So, it
> will be no matches when looking for the subscription to delete later.
> 
> The subscription(s) will be removed eventually when the user terminates
> its topology connection but that could be a long time later. Meanwhile,
> the number of available subscriptions may be exhausted.
> 
> This commit fixes the two issues above, so as needed a subscription can
> be deleted correctly.
> 
> v2: define a new macro to write sub field value (- Jon's comment)
> v3: break if the sub to be deleted has been found
> 
> Signed-off-by: Tuong Lien 

Acked-by: Ying Xue 

> ---
>  net/tipc/subscr.h | 10 ++
>  net/tipc/topsrv.c |  9 +
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/net/tipc/subscr.h b/net/tipc/subscr.h
> index aa015c233898..6ebbec1bedd1 100644
> --- a/net/tipc/subscr.h
> +++ b/net/tipc/subscr.h
> @@ -96,6 +96,16 @@ void tipc_sub_get(struct tipc_subscription *subscription);
>   (swap_ ? swab32(val__) : val__);\
>   })
>  
> +/* tipc_sub_write - write val_ to field_ of struct sub_ in user endian format
> + */
> +#define tipc_sub_write(sub_, field_, val_)   \
> + ({  \
> + struct tipc_subscr *sub__ = sub_;   \
> + u32 val__ = val_;   \
> + int swap_ = !((sub__)->filter & TIPC_FILTER_MASK);  \
> + (sub__)->field_ = swap_ ? swab32(val__) : val__;\
> + })
> +
>  /* tipc_evt_write - write val_ to field_ of struct evt_ in user endian format
>   */
>  #define tipc_evt_write(evt_, field_, val_)   \
> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index 931c426673c0..446af7bbd13e 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c
> @@ -237,8 +237,8 @@ static void tipc_conn_delete_sub(struct tipc_conn *con, 
> struct tipc_subscr *s)
>   if (!s || !memcmp(s, >evt.s, sizeof(*s))) {
>   tipc_sub_unsubscribe(sub);
>   atomic_dec(>subscription_count);
> - } else if (s) {
> - break;
> + if (s)
> + break;
>   }
>   }
>   spin_unlock_bh(>sub_lock);
> @@ -362,9 +362,10 @@ static int tipc_conn_rcv_sub(struct tipc_topsrv *srv,
>  {
>   struct tipc_net *tn = tipc_net(srv->net);
>   struct tipc_subscription *sub;
> + u32 s_filter = tipc_sub_read(s, filter);
>  
> - if (tipc_sub_read(s, filter) & TIPC_SUB_CANCEL) {
> - s->filter &= __constant_ntohl(~TIPC_SUB_CANCEL);
> + if (s_filter & TIPC_SUB_CANCEL) {
> + tipc_sub_write(s, filter, s_filter & ~TIPC_SUB_CANCEL);
>   tipc_conn_delete_sub(con, s);
>   return 0;
>   }
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net 2/2] tipc: fix failed service subscription deletion

[Ying Xue]

On 5/7/20 7:12 PM, Tuong Lien wrote:
> When a service subscription is expired or canceled by user, it needs to
> be deleted from the subscription list, so that new subscriptions can be
> registered (max = 65535 per net). However, there are two issues in code
> that can cause such an unused subscription to persist:
> 
> 1) The 'tipc_conn_delete_sub()' has a loop on the subscription list but
> it makes a break shortly when the 1st subscription differs from the one
> specified, so the subscription will not be deleted.
> 
> 2) In case a subscription is canceled, the code to remove the
> 'TIPC_SUB_CANCEL' flag from the subscription filter does not work if it
> is a local subscription (i.e. the little endian isn't involved). So, it
> will be no matches when looking for the subscription to delete later.
> 
> The subscription(s) will be removed eventually when the user terminates
> its topology connection but that could be a long time later. Meanwhile,
> the number of available subscriptions may be exhausted.
> 
> This commit fixes the two issues above, so as needed a subscription can
> be deleted correctly.
> 
> Signed-off-by: Tuong Lien 
> ---
>  net/tipc/topsrv.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index 399a89f6f1bf..44609238849e 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c
> @@ -237,8 +237,6 @@ static void tipc_conn_delete_sub(struct tipc_conn *con, 
> struct tipc_subscr *s)
>   if (!s || !memcmp(s, >evt.s, sizeof(*s))) {
>   tipc_sub_unsubscribe(sub);
>   atomic_dec(>subscription_count);
> - } else if (s) {
> - break;

I am quite surprised at this issue. The issue really exists as you
describe above, but it's better to fix it as follows:

@@ -237,8 +237,8 @@ static void tipc_conn_delete_sub(struct tipc_conn
*con, struct tipc_subscr *s)
if (!s || !memcmp(s, >evt.s, sizeof(*s))) {
tipc_sub_unsubscribe(sub);
atomic_dec(>subscription_count);
-   } else if (s) {
-   break;
+   if (s)
+   break;
}
}

>   }
>   }
>   spin_unlock_bh(>sub_lock);
> @@ -364,7 +362,10 @@ static int tipc_conn_rcv_sub(struct tipc_topsrv *srv,
>   struct tipc_subscription *sub;
>  
>   if (tipc_sub_read(s, filter) & TIPC_SUB_CANCEL) {
> - s->filter &= __constant_ntohl(~TIPC_SUB_CANCEL);
> + if (!(s->filter & TIPC_FILTER_MASK))
> + s->filter &= __constant_ntohl(~TIPC_SUB_CANCEL);
> + else
> + s->filter &= ~TIPC_SUB_CANCEL;
>   tipc_conn_delete_sub(con, s);
>   return 0;
>   }
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] TIPC_WITHDRAW event comes late

[Ying Xue]

Please validate whether it appears with the latest kernel.

If you cannot, please provide more detailed info:

1. Capture TIPC packets with tcpdump tool;
2. Provide dmesg log, but be sure each line of demeg logs should be
prefixed by timestamps.
3. Print timestamp as well when you receive TIPC_WITHDRAW on user land.


On 5/8/20 11:01 AM, Hung Ta wrote:
> Hi TIPC experts,
> 
> I'm using the TIPC library in my project which needs to be aware of a node
> leaves the cluster.
> 
> So, I use socket type AF_TIPC and SOCK_SEQPACKET and connect it
> to TIPC_TOP_SRV.
> 
> I tried to get several nodes up and then make one of them leave and then I
> can see the event TIPC_WITHDRAW seen, but the issue is it comes very late,
> in particular,  it comes about 10 seconds late after a node left the
> cluster.
> I'm using Ubuntu 16.04, kernel 4.4.0-87-generic.
> The same issue also occurs in Ubuntu 14.04.
> 
> Why is it too late?
> Appreciate your help.
> 
> Thanks and regards,
> Hung
> 
> ___
> tipc-discussion mailing list
> tipc-discussion@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net 1/2] tipc: fix memory leak in service subscripting

[Ying Xue]

Good catch!

On 5/7/20 7:12 PM, Tuong Lien wrote:
> Upon receipt of a service subscription request from user via a topology
> connection, one 'sub' object will be allocated in kernel, so it will be
> able to send an event of the service if any to the user correspondingly
> then. Also, in case of any failure, the connection will be shutdown and
> all the pertaining 'sub' objects will be freed.
> 
> However, there is a race condition as follows resulting in memory leak:
> 
>receive-work   connectionsend-work
>   |||
> sub-1 |<--//---||
> sub-2 |<--//---||
>   ||<---| evt for sub-x
> sub-3 |<--//---||
>   :::
>   :::
>   |   /||
>   |   |* peer closed|
>   |   |||
>   |   ||<---X---| evt for sub-y
>   |   ||<===|
> sub-n |<--/Xshutdown|
> -> orphan | |
> 
> That is, the 'receive-work' may get the last subscription request while
> the 'send-work' is shutting down the connection due to peer close.
> 
> We had a 'lock' on the connection, so the two actions cannot be carried
> out simultaneously. If the last subscription is allocated e.g. 'sub-n',
> before the 'send-work' closes the connection, there will be no issue at
> all, the 'sub' objects will be freed. In contrast the last subscription
> will become orphan since the connection was closed, and we released all
> references.
> 
> This commit fixes the issue by simply adding one test if the connection
> remains in 'connected' state soon after we obtain the connection's lock
> then a subscription object can be created as usual, otherwise we ignore
> it.
> 
> Reported-by: Thang Ngo 
> Signed-off-by: Tuong Lien 

Acked-by: Ying Xue 

> ---
>  net/tipc/topsrv.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index 73dbed0c4b6b..399a89f6f1bf 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c
> @@ -400,7 +400,9 @@ static int tipc_conn_rcv_from_sock(struct tipc_conn *con)
>   return -EWOULDBLOCK;
>   if (ret == sizeof(s)) {
>   read_lock_bh(>sk_callback_lock);
> - ret = tipc_conn_rcv_sub(srv, con, );
> + /* RACE: the connection can be closed in meanwhile */
> + if (likely(connected(con)))
> + ret = tipc_conn_rcv_sub(srv, con, );
>   read_unlock_bh(>sk_callback_lock);
>   if (!ret)
>   return 0;
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [RFC PATCH 1/2] tipc: fix large latency in smart Nagle streaming

[Ying Xue]

> You mean for SOCK_SEQPACKET? But we don't apply smart Nagle to that sock type 
> (only SOCK_STREAM), nor is there such code in tipc_recvmsg().

Thanks for the reminder. Yes, you are right. Please ignore my previous
comment.


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net] tipc: fix partial topology connection closure

[Ying Xue]

On 4/28/20 4:58 PM, Tuong Lien wrote:
> When an application connects to the TIPC topology server and subscribes
> to some services, a new connection is created along with some objects -
> 'tipc_subscription' to store related data correspondingly...
> However, there is one omission in the connection handling that when the
> connection or application is orderly shutdown (e.g. via SIGQUIT, etc.),
> the connection is not closed in kernel, the 'tipc_subscription' objects
> are not freed too.
> This results in:
> - The maximum number of subscriptions (65535) will be reached soon, new
> subscriptions will be rejected;
> - TIPC module cannot be removed (unless the objectes are somehow forced
> to release first);
> 
> The commit fixes the issue by closing the connection if the 'recvmsg()'
> returns '0' i.e. when the peer is shutdown gracefully. It also includes
> the other unexpected cases.
> 
> Signed-off-by: Tuong Lien 

Acked-by: Ying Xue 

> ---
>  net/tipc/topsrv.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index ad78f7cff379..c364335623ab 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c
> @@ -405,10 +405,11 @@ static int tipc_conn_rcv_from_sock(struct tipc_conn 
> *con)
>   read_lock_bh(>sk_callback_lock);
>   ret = tipc_conn_rcv_sub(srv, con, );
>   read_unlock_bh(>sk_callback_lock);
> + if (!ret)
> + return 0;
>   }
> - if (ret < 0)
> - tipc_conn_close(con);
>  
> + tipc_conn_close(con);
>   return ret;
>  }
>  
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [PATCH net] tipc: eliminate KMSAN: uninit-value in __tipc_nl_compat_dumpit error

[Ying Xue]

syzbot found the following crash on:
=
BUG: KMSAN: uninit-value in __nlmsg_parse include/net/netlink.h:661 [inline]
BUG: KMSAN: uninit-value in nlmsg_parse_deprecated
include/net/netlink.h:706 [inline]
BUG: KMSAN: uninit-value in __tipc_nl_compat_dumpit+0x553/0x11e0
net/tipc/netlink_compat.c:215
CPU: 0 PID: 12425 Comm: syz-executor062 Not tainted 5.5.0-rc1-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x1c9/0x220 lib/dump_stack.c:118
  kmsan_report+0x128/0x220 mm/kmsan/kmsan_report.c:108
  __msan_warning+0x57/0xa0 mm/kmsan/kmsan_instr.c:245
  __nlmsg_parse include/net/netlink.h:661 [inline]
  nlmsg_parse_deprecated include/net/netlink.h:706 [inline]
  __tipc_nl_compat_dumpit+0x553/0x11e0 net/tipc/netlink_compat.c:215
  tipc_nl_compat_dumpit+0x761/0x910 net/tipc/netlink_compat.c:308
  tipc_nl_compat_handle net/tipc/netlink_compat.c:1252 [inline]
  tipc_nl_compat_recv+0x12e9/0x2870 net/tipc/netlink_compat.c:1311
  genl_family_rcv_msg_doit net/netlink/genetlink.c:672 [inline]
  genl_family_rcv_msg net/netlink/genetlink.c:717 [inline]
  genl_rcv_msg+0x1dd0/0x23a0 net/netlink/genetlink.c:734
  netlink_rcv_skb+0x431/0x620 net/netlink/af_netlink.c:2477
  genl_rcv+0x63/0x80 net/netlink/genetlink.c:745
  netlink_unicast_kernel net/netlink/af_netlink.c:1302 [inline]
  netlink_unicast+0xfa0/0x1100 net/netlink/af_netlink.c:1328
  netlink_sendmsg+0x11f0/0x1480 net/netlink/af_netlink.c:1917
  sock_sendmsg_nosec net/socket.c:639 [inline]
  sock_sendmsg net/socket.c:659 [inline]
  sys_sendmsg+0x1362/0x13f0 net/socket.c:2330
  ___sys_sendmsg net/socket.c:2384 [inline]
  __sys_sendmsg+0x4f0/0x5e0 net/socket.c:2417
  __do_sys_sendmsg net/socket.c:2426 [inline]
  __se_sys_sendmsg+0x97/0xb0 net/socket.c:2424
  __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2424
  do_syscall_64+0xb6/0x160 arch/x86/entry/common.c:295
  entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x444179
Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 1b d8 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:7ffd2d6409c8 EFLAGS: 0246 ORIG_RAX: 002e
RAX: ffda RBX: 004002e0 RCX: 00444179
RDX:  RSI: 2140 RDI: 0003
RBP: 006ce018 R08:  R09: 004002e0
R10:  R11: 0246 R12: 00401e20
R13: 00401eb0 R14:  R15: 

Uninit was created at:
  kmsan_save_stack_with_flags mm/kmsan/kmsan.c:149 [inline]
  kmsan_internal_poison_shadow+0x5c/0x110 mm/kmsan/kmsan.c:132
  kmsan_slab_alloc+0x8a/0xe0 mm/kmsan/kmsan_hooks.c:86
  slab_alloc_node mm/slub.c:2774 [inline]
  __kmalloc_node_track_caller+0xe47/0x11f0 mm/slub.c:4382
  __kmalloc_reserve net/core/skbuff.c:141 [inline]
  __alloc_skb+0x309/0xa50 net/core/skbuff.c:209
  alloc_skb include/linux/skbuff.h:1049 [inline]
  nlmsg_new include/net/netlink.h:888 [inline]
  tipc_nl_compat_dumpit+0x6e4/0x910 net/tipc/netlink_compat.c:301
  tipc_nl_compat_handle net/tipc/netlink_compat.c:1252 [inline]
  tipc_nl_compat_recv+0x12e9/0x2870 net/tipc/netlink_compat.c:1311
  genl_family_rcv_msg_doit net/netlink/genetlink.c:672 [inline]
  genl_family_rcv_msg net/netlink/genetlink.c:717 [inline]
  genl_rcv_msg+0x1dd0/0x23a0 net/netlink/genetlink.c:734
  netlink_rcv_skb+0x431/0x620 net/netlink/af_netlink.c:2477
  genl_rcv+0x63/0x80 net/netlink/genetlink.c:745
  netlink_unicast_kernel net/netlink/af_netlink.c:1302 [inline]
  netlink_unicast+0xfa0/0x1100 net/netlink/af_netlink.c:1328
  netlink_sendmsg+0x11f0/0x1480 net/netlink/af_netlink.c:1917
  sock_sendmsg_nosec net/socket.c:639 [inline]
  sock_sendmsg net/socket.c:659 [inline]
  sys_sendmsg+0x1362/0x13f0 net/socket.c:2330
  ___sys_sendmsg net/socket.c:2384 [inline]
  __sys_sendmsg+0x4f0/0x5e0 net/socket.c:2417
  __do_sys_sendmsg net/socket.c:2426 [inline]
  __se_sys_sendmsg+0x97/0xb0 net/socket.c:2424
  __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2424
  do_syscall_64+0xb6/0x160 arch/x86/entry/common.c:295
  entry_SYSCALL_64_after_hwframe+0x44/0xa9
=

The complaint above occurred because the memory region pointed by attrbuf
variable was not initialized. To eliminate this warning, we use kcalloc()
rather than kmalloc_array() to allocate memory for attrbuf.

Reported-by: syzbot+b1fd2bf2c89d8407e...@syzkaller.appspotmail.com
Signed-off-by: Ying Xue 
---
 net/tipc/netlink_compat.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index 0254bb7..2175163 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -204,8 +204,8 @@ static int __tipc_nl_compat_dumpit(struct 
tipc_nl_compat_cm

Re: [tipc-discussion] [net] tipc: fix wrong connect() return code

[Ying Xue]

On 12/27/19 10:22 AM, Tuong Lien Tong wrote:
> Hi Ying,
> 
> Thinking more about this...
> I suppose we can even remove the function and utilize the generic macro
> 'tipc_wait_for_cond()' but with the new condition, that is much simpler and
> also saves some footprint.

Agreed.

> The 'tipc_sk_connected()' condition is really what we should expect in this
> case instead.

Yes.

> So, in the case of 'TIPC_DISCONNECTING', regardless of whether we have a
> 'sk->sk_err', it will return that error or '-ETIMEOUT' but never '0'.

But I think it's better to return the error attached on 'sk->sk_err' to
user because it can really reflect why connect() is failed.

> I will send the patch v2 for your review.
> 
> BR/Tuong
> 
> -Original Message-
> From: Tuong Lien Tong  
> Sent: Thursday, December 26, 2019 5:39 PM
> To: 'Ying Xue' ;
> tipc-discussion@lists.sourceforge.net; jon.ma...@ericsson.com;
> ma...@donjonn.com
> Subject: Re: [tipc-discussion] [net] tipc: fix wrong connect() return code
> 
> What about this?
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index 6ebd809ef207..04f035bcc272 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -2446,7 +2446,7 @@ static int tipc_wait_for_connect(struct socket *sock,
> long *timeo_p)
> done = sk_wait_event(sk, timeo_p,
>  sk->sk_state != TIPC_CONNECTING,
> );
> remove_wait_queue(sk_sleep(sk), );
> -   } while (!done);
> +   } while (!tipc_sk_connected(sk));
> return 0;
>  }
> 
> BR/Tuong
> 
> -Original Message-
> From: Ying Xue  
> Sent: Thursday, December 26, 2019 2:55 PM
> To: Tuong Lien Tong ;
> tipc-discussion@lists.sourceforge.net; jon.ma...@ericsson.com;
> ma...@donjonn.com
> Subject: Re: [net] tipc: fix wrong connect() return code
> 
> On 12/26/19 3:16 PM, Tuong Lien Tong wrote:
>> Sorry, my mistake! I thought it was "while (!done || !sk->sk_err)"...
>> But, this is really confusing and one might ask why we continue the loop
> while the socket has encountered an error (sk-> sk_err! = 0)???
> 
> This is totally reasonable because it can make code simpler.
> [Tuong]: but a performance hit since it almost looks like the sk_err has to
> be read twice?
> 
>> Moreover, it's not necessarily the "sk_err" will be the only case,
>> For example:
>>  sk->sk_state != TIPC_CONNECTING but sk->sk_err = 0 (somehow) and a
> pending/interrupt signal
> 
> If sk->sk_state is changed from TIPC_CONNECTING to TIPC_ESTABLISHED,
> sk->sk_err should be kept 0, but if sk->sk_state is changed to other
> states, sk->sk_err must be set with a proper error code in socket
> receive path, otherwise, it's a bug.
> 
> However, there might be one below race condition:
> 
> sk->sk_state is set to TIPC_DISCONNECTING, but before sk->sk_err is set
> with an error code, the process who calls connect() is woken up by one
> single. Even so, the process is still blocked when it tries to hold sock
> lock with lock_sock() in sk_wait_event() because the socket lock is
> taken in socket receive path. After socket lock is released, the process
> will continue being run. But at that moment, sk->sk_err has been set
> with an error code. So, this is no problem for us.
> 
>>
>> then we should return '-EINTR'?
> 
> 
> 
> ___
> tipc-discussion mailing list
> tipc-discussion@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
> 
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [PATCH] tipc: eliminate KMSAN: uninit-value in __tipc_nl_compat_dumpit error

[Ying Xue]

syzbot found the following crash on:
=
BUG: KMSAN: uninit-value in __nlmsg_parse include/net/netlink.h:661 [inline]
BUG: KMSAN: uninit-value in nlmsg_parse_deprecated
include/net/netlink.h:706 [inline]
BUG: KMSAN: uninit-value in __tipc_nl_compat_dumpit+0x553/0x11e0
net/tipc/netlink_compat.c:215
CPU: 0 PID: 12425 Comm: syz-executor062 Not tainted 5.5.0-rc1-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x1c9/0x220 lib/dump_stack.c:118
  kmsan_report+0x128/0x220 mm/kmsan/kmsan_report.c:108
  __msan_warning+0x57/0xa0 mm/kmsan/kmsan_instr.c:245
  __nlmsg_parse include/net/netlink.h:661 [inline]
  nlmsg_parse_deprecated include/net/netlink.h:706 [inline]
  __tipc_nl_compat_dumpit+0x553/0x11e0 net/tipc/netlink_compat.c:215
  tipc_nl_compat_dumpit+0x761/0x910 net/tipc/netlink_compat.c:308
  tipc_nl_compat_handle net/tipc/netlink_compat.c:1252 [inline]
  tipc_nl_compat_recv+0x12e9/0x2870 net/tipc/netlink_compat.c:1311
  genl_family_rcv_msg_doit net/netlink/genetlink.c:672 [inline]
  genl_family_rcv_msg net/netlink/genetlink.c:717 [inline]
  genl_rcv_msg+0x1dd0/0x23a0 net/netlink/genetlink.c:734
  netlink_rcv_skb+0x431/0x620 net/netlink/af_netlink.c:2477
  genl_rcv+0x63/0x80 net/netlink/genetlink.c:745
  netlink_unicast_kernel net/netlink/af_netlink.c:1302 [inline]
  netlink_unicast+0xfa0/0x1100 net/netlink/af_netlink.c:1328
  netlink_sendmsg+0x11f0/0x1480 net/netlink/af_netlink.c:1917
  sock_sendmsg_nosec net/socket.c:639 [inline]
  sock_sendmsg net/socket.c:659 [inline]
  sys_sendmsg+0x1362/0x13f0 net/socket.c:2330
  ___sys_sendmsg net/socket.c:2384 [inline]
  __sys_sendmsg+0x4f0/0x5e0 net/socket.c:2417
  __do_sys_sendmsg net/socket.c:2426 [inline]
  __se_sys_sendmsg+0x97/0xb0 net/socket.c:2424
  __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2424
  do_syscall_64+0xb6/0x160 arch/x86/entry/common.c:295
  entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x444179
Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 1b d8 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:7ffd2d6409c8 EFLAGS: 0246 ORIG_RAX: 002e
RAX: ffda RBX: 004002e0 RCX: 00444179
RDX:  RSI: 2140 RDI: 0003
RBP: 006ce018 R08:  R09: 004002e0
R10:  R11: 0246 R12: 00401e20
R13: 00401eb0 R14:  R15: 

Uninit was created at:
  kmsan_save_stack_with_flags mm/kmsan/kmsan.c:149 [inline]
  kmsan_internal_poison_shadow+0x5c/0x110 mm/kmsan/kmsan.c:132
  kmsan_slab_alloc+0x8a/0xe0 mm/kmsan/kmsan_hooks.c:86
  slab_alloc_node mm/slub.c:2774 [inline]
  __kmalloc_node_track_caller+0xe47/0x11f0 mm/slub.c:4382
  __kmalloc_reserve net/core/skbuff.c:141 [inline]
  __alloc_skb+0x309/0xa50 net/core/skbuff.c:209
  alloc_skb include/linux/skbuff.h:1049 [inline]
  nlmsg_new include/net/netlink.h:888 [inline]
  tipc_nl_compat_dumpit+0x6e4/0x910 net/tipc/netlink_compat.c:301
  tipc_nl_compat_handle net/tipc/netlink_compat.c:1252 [inline]
  tipc_nl_compat_recv+0x12e9/0x2870 net/tipc/netlink_compat.c:1311
  genl_family_rcv_msg_doit net/netlink/genetlink.c:672 [inline]
  genl_family_rcv_msg net/netlink/genetlink.c:717 [inline]
  genl_rcv_msg+0x1dd0/0x23a0 net/netlink/genetlink.c:734
  netlink_rcv_skb+0x431/0x620 net/netlink/af_netlink.c:2477
  genl_rcv+0x63/0x80 net/netlink/genetlink.c:745
  netlink_unicast_kernel net/netlink/af_netlink.c:1302 [inline]
  netlink_unicast+0xfa0/0x1100 net/netlink/af_netlink.c:1328
  netlink_sendmsg+0x11f0/0x1480 net/netlink/af_netlink.c:1917
  sock_sendmsg_nosec net/socket.c:639 [inline]
  sock_sendmsg net/socket.c:659 [inline]
  sys_sendmsg+0x1362/0x13f0 net/socket.c:2330
  ___sys_sendmsg net/socket.c:2384 [inline]
  __sys_sendmsg+0x4f0/0x5e0 net/socket.c:2417
  __do_sys_sendmsg net/socket.c:2426 [inline]
  __se_sys_sendmsg+0x97/0xb0 net/socket.c:2424
  __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2424
  do_syscall_64+0xb6/0x160 arch/x86/entry/common.c:295
  entry_SYSCALL_64_after_hwframe+0x44/0xa9
=

The complaint above occurred because the memory region pointed by attrbuf
variable was not initialized. To eliminate this warning, we use kcalloc()
rather than kmalloc_array() to allocate memory for attrbuf.

Reported-by: syzbot+b1fd2bf2c89d8407e...@syzkaller.appspotmail.com
Signed-off-by: Ying Xue 
---
 net/tipc/netlink_compat.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index 0254bb7..2175163 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -204,8 +204,8 @@ static int __tipc_nl_compat_dumpit(struct 
tipc_nl_compat_cm

Re: [tipc-discussion] [net] tipc: fix wrong connect() return code

[Ying Xue]

On 12/26/19 3:16 PM, Tuong Lien Tong wrote:
> Sorry, my mistake! I thought it was "while (!done || !sk->sk_err)"...
> But, this is really confusing and one might ask why we continue the loop 
> while the socket has encountered an error (sk-> sk_err! = 0)???

This is totally reasonable because it can make code simpler.

> Moreover, it's not necessarily the "sk_err" will be the only case,
> For example:
>   sk->sk_state != TIPC_CONNECTING but sk->sk_err = 0 (somehow) and a 
> pending/interrupt signal

If sk->sk_state is changed from TIPC_CONNECTING to TIPC_ESTABLISHED,
sk->sk_err should be kept 0, but if sk->sk_state is changed to other
states, sk->sk_err must be set with a proper error code in socket
receive path, otherwise, it's a bug.

However, there might be one below race condition:

sk->sk_state is set to TIPC_DISCONNECTING, but before sk->sk_err is set
with an error code, the process who calls connect() is woken up by one
single. Even so, the process is still blocked when it tries to hold sock
lock with lock_sock() in sk_wait_event() because the socket lock is
taken in socket receive path. After socket lock is released, the process
will continue being run. But at that moment, sk->sk_err has been set
with an error code. So, this is no problem for us.

> 
> then we should return '-EINTR'?


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net] tipc: fix wrong connect() return code

[Ying Xue]

On 12/26/19 9:04 AM, Tuong Lien Tong wrote:
> Hi Ying,
> 
> Actually, the 'done' flag has been set in the particular case (since the
> 'sk->sk_state' changed to 'TIPC_DISCONNECTING' after receiving a rejection
> of its SYN from server...) and what we want to achieve is the error code
> from the 'sock_error(sk)' to be returned to user correctly.
> For your code, there is no difference, the function would still return '0'
> for the said case.

--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2435,7 +2435,7 @@ static int tipc_wait_for_connect(struct socket *sock,
long *timeo_p)
done = sk_wait_event(sk, timeo_p,
 sk->sk_state != TIPC_CONNECTING,
);
remove_wait_queue(sk_sleep(sk), );
-   } while (!done);
+   } while (!done || sk->sk_err);
return 0;
 }

Sorry, in my understanding, if this case you mentioned above occurs,
"done" will be really set to 1, which means "while loop" will exit and
then 0 will be returned to 0. This is our current problem.

But, if we add "sk->sk_err" as another while statement condition, the
while loop will not exit because "sk->sk_err" is not 0. As a
consequence, in next loop, sock error code will be returned to user
because sock_error() is not 0.

> I considered an alternative that:
> 
> done = sk_wait_event(sk, timeo_p,
>  sk->sk_state != TIPC_CONNECTING,
> );
> remove_wait_queue(sk_sleep(sk), );
> } while (!done);
> -   return 0;
> +  return sock_err(sk);
> 
> but this could get more concerns or we should check the socket state at the
> return e.g. 
> return (sk->sk_state != TIPC_ESTABLISHED) ? sock_err(sk) : 0;
> 
> and the fact is that we have this code already in the while statement, so I
> have decided to go with the code below.


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH net/tipc] Replace rcu_swap_protected() with rcu_replace_pointer()

[Ying Xue]

On 12/12/19 2:46 AM, Paul E. McKenney wrote:
> On Wed, Dec 11, 2019 at 12:42:00PM +0800, Ying Xue wrote:
>> On 12/11/19 10:00 AM, Tuong Lien Tong wrote:
>>>>  
>>>>/* Move passive key if any */
>>>>if (key.passive) {
>>>> -  tipc_aead_rcu_swap(rx->aead[key.passive], tmp2, >lock);
>>>> +  tmp2 = rcu_replace_pointer(rx->aead[key.passive], tmp2,
>>> >lock);
>>> The 3rd parameter should be the lockdep condition checking instead of the
>>> spinlock's pointer i.e. "lockdep_is_held(>lock)"?
>>> That's why I'd prefer to use the 'tipc_aead_rcu_swap ()' macro, which is
>>> clear & concise at least for the context here. It might be re-used later as
>>> well...
>>>
>>
>> Right. The 3rd parameter of rcu_replace_pointer() should be
>> "lockdep_is_held(>lock)" instead of ">lock".
> 
> Like this?

Yes, I think it's better to set the 3rd parameter of
rcu_replace_pointer() with "lockdep_is_held(>lock)".

> 
>   Thanx, Paul
> 
> 
> 
> commit 575bb4ba1b22383656760feb3d122e11656ccdfd
> Author: Paul E. McKenney 
> Date:   Mon Dec 9 19:13:45 2019 -0800
> 
> net/tipc: Replace rcu_swap_protected() with rcu_replace_pointer()
> 
> This commit replaces the use of rcu_swap_protected() with the more
> intuitively appealing rcu_replace_pointer() as a step towards removing
> rcu_swap_protected().
>     
> Link: 
> https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=z7-ggtm6wcvtyytxza1+bhqta4gg...@mail.gmail.com/
> Reported-by: Linus Torvalds 
> Reported-by: kbuild test robot 
> Signed-off-by: Paul E. McKenney 
> [ paulmck: Updated based on Ying Xue and Tuong Lien Tong feedback. ]
> Cc: Jon Maloy 
> Cc: Ying Xue 
> Cc: "David S. Miller" 
> Cc: 
> Cc: 
> 
> diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
> index 990a872..c8c47fc 100644
> --- a/net/tipc/crypto.c
> +++ b/net/tipc/crypto.c
> @@ -257,9 +257,6 @@ static char *tipc_key_change_dump(struct tipc_key old, 
> struct tipc_key new,
>  #define tipc_aead_rcu_ptr(rcu_ptr, lock) \
>   rcu_dereference_protected((rcu_ptr), lockdep_is_held(lock))
>  
> -#define tipc_aead_rcu_swap(rcu_ptr, ptr, lock)   
> \
> - rcu_swap_protected((rcu_ptr), (ptr), lockdep_is_held(lock))
> -
>  #define tipc_aead_rcu_replace(rcu_ptr, ptr, lock)\
>  do { \
>   typeof(rcu_ptr) __tmp = rcu_dereference_protected((rcu_ptr),\
> @@ -1189,7 +1186,7 @@ static bool tipc_crypto_key_try_align(struct 
> tipc_crypto *rx, u8 new_pending)
>  
>   /* Move passive key if any */
>   if (key.passive) {
> - tipc_aead_rcu_swap(rx->aead[key.passive], tmp2, >lock);
> + tmp2 = rcu_replace_pointer(rx->aead[key.passive], tmp2, 
> lockdep_is_held(>lock));
>   x = (key.passive - key.pending + new_pending) % KEY_MAX;
>   new_passive = (x <= 0) ? x + KEY_MAX : x;
>   }
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH net/tipc] Replace rcu_swap_protected() with rcu_replace_pointer()

[Ying Xue]

On 12/11/19 10:00 AM, Tuong Lien Tong wrote:
>>  
>>  /* Move passive key if any */
>>  if (key.passive) {
>> -tipc_aead_rcu_swap(rx->aead[key.passive], tmp2, >lock);
>> +tmp2 = rcu_replace_pointer(rx->aead[key.passive], tmp2,
> >lock);
> The 3rd parameter should be the lockdep condition checking instead of the
> spinlock's pointer i.e. "lockdep_is_held(>lock)"?
> That's why I'd prefer to use the 'tipc_aead_rcu_swap ()' macro, which is
> clear & concise at least for the context here. It might be re-used later as
> well...
> 

Right. The 3rd parameter of rcu_replace_pointer() should be
"lockdep_is_held(>lock)" instead of ">lock".


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH net/tipc] Replace rcu_swap_protected() with rcu_replace_pointer()

[Ying Xue]

On 12/11/19 11:17 AM, Paul E. McKenney wrote:
>> Acked-by: Ying Xue 
> As in the following?  If so, I will be very happy to apply your Acked-by.

Yes. Thanks!


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH net/tipc] Replace rcu_swap_protected() with rcu_replace_pointer()

[Ying Xue]

On 12/11/19 6:38 AM, Paul E. McKenney wrote:
> commit 4ee8e2c68b076867b7a5af82a38010fffcab611c
> Author: Paul E. McKenney 
> Date:   Mon Dec 9 19:13:45 2019 -0800
> 
> net/tipc: Replace rcu_swap_protected() with rcu_replace_pointer()
> 
> This commit replaces the use of rcu_swap_protected() with the more
> intuitively appealing rcu_replace_pointer() as a step towards removing
> rcu_swap_protected().
> 
> Link: 
> https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=z7-ggtm6wcvtyytxza1+bhqta4gg...@mail.gmail.com/
> Reported-by: Linus Torvalds 
> Reported-by: kbuild test robot 
> Signed-off-by: Paul E. McKenney 
> Cc: Jon Maloy 
> Cc: Ying Xue 
> Cc: "David S. Miller" 
> Cc: 
> Cc: 
> 

Acked-by: Ying Xue 

> diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
> index 990a872..978d2db 100644
> --- a/net/tipc/crypto.c
> +++ b/net/tipc/crypto.c
> @@ -257,9 +257,6 @@ static char *tipc_key_change_dump(struct tipc_key old, 
> struct tipc_key new,
>  #define tipc_aead_rcu_ptr(rcu_ptr, lock) \
>   rcu_dereference_protected((rcu_ptr), lockdep_is_held(lock))
>  
> -#define tipc_aead_rcu_swap(rcu_ptr, ptr, lock)   
> \
> - rcu_swap_protected((rcu_ptr), (ptr), lockdep_is_held(lock))
> -
>  #define tipc_aead_rcu_replace(rcu_ptr, ptr, lock)\
>  do { \
>   typeof(rcu_ptr) __tmp = rcu_dereference_protected((rcu_ptr),\
> @@ -1189,7 +1186,7 @@ static bool tipc_crypto_key_try_align(struct 
> tipc_crypto *rx, u8 new_pending)
>  
>   /* Move passive key if any */
>   if (key.passive) {
> - tipc_aead_rcu_swap(rx->aead[key.passive], tmp2, >lock);
> + tmp2 = rcu_replace_pointer(rx->aead[key.passive], tmp2, 
> >lock);
>   x = (key.passive - key.pending + new_pending) % KEY_MAX;
>   new_passive = (x <= 0) ? x + KEY_MAX : x;
>   }
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH net/tipc] Replace rcu_swap_protected() with rcu_replace_pointer()

[Ying Xue]

On 12/10/19 11:31 AM, Paul E. McKenney wrote:
> This commit replaces the use of rcu_swap_protected() with the more
> intuitively appealing rcu_replace_pointer() as a step towards removing
> rcu_swap_protected().
> 
> Link: 
> https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=z7-ggtm6wcvtyytxza1+bhqta4gg...@mail.gmail.com/
> Reported-by: Linus Torvalds 
> Reported-by: kbuild test robot 
> Signed-off-by: Paul E. McKenney 
> Cc: Jon Maloy 
> Cc: Ying Xue 
> Cc: "David S. Miller" 
> Cc: 
> Cc: 
> 
> diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
> index 990a872..64cf831 100644
> --- a/net/tipc/crypto.c
> +++ b/net/tipc/crypto.c
> @@ -258,7 +258,7 @@ static char *tipc_key_change_dump(struct tipc_key old, 
> struct tipc_key new,
>   rcu_dereference_protected((rcu_ptr), lockdep_is_held(lock))
>  
>  #define tipc_aead_rcu_swap(rcu_ptr, ptr, lock)   
> \
> - rcu_swap_protected((rcu_ptr), (ptr), lockdep_is_held(lock))
> + rcu_replace_pointer((rcu_ptr), (ptr), lockdep_is_held(lock))

(ptr) = rcu_replace_pointer((rcu_ptr), (ptr), lockdep_is_held(lock))

>  
>  #define tipc_aead_rcu_replace(rcu_ptr, ptr, lock)\
>  do { \
> @@ -1189,7 +1189,7 @@ static bool tipc_crypto_key_try_align(struct 
> tipc_crypto *rx, u8 new_pending)
>  
>   /* Move passive key if any */
>   if (key.passive) {
> - tipc_aead_rcu_swap(rx->aead[key.passive], tmp2, >lock);
> + tmp2 = rcu_replace_pointer(rx->aead[key.passive], tmp2, 
> >lock);

tipc_aead_rcu_swap() is only called here in TIPC module. If we use
rcu_replace_pointer() to switch pointers instead of calling
tipc_aead_rcu_swap() macro, I think we should completely remove
tipc_aead_rcu_swap().

>   x = (key.passive - key.pending + new_pending) % KEY_MAX;
>   new_passive = (x <= 0) ? x + KEY_MAX : x;
>   }
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net] tipc: fix use-after-free in tipc_disc_rcv()

[Ying Xue]

On 12/9/19 6:11 PM, Tuong Lien wrote:
> In the function 'tipc_disc_rcv()', the 'msg_peer_net_hash()' is called
> to read the header data field but after the message skb has been freed,
> that might result in a garbage value...
> 
> This commit fixes it by defining a new local variable to store the data
> first, just like the other header fields' handling.
> 
> Fixes: f73b12812a3d ("tipc: improve throughput between nodes in netns")
> Signed-off-by: Tuong Lien 

Acked-by: Ying Xue 

> ---
>  net/tipc/discover.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/tipc/discover.c b/net/tipc/discover.c
> index b043e8c6397a..bfe43da127c0 100644
> --- a/net/tipc/discover.c
> +++ b/net/tipc/discover.c
> @@ -194,6 +194,7 @@ void tipc_disc_rcv(struct net *net, struct sk_buff *skb,
>  {
>   struct tipc_net *tn = tipc_net(net);
>   struct tipc_msg *hdr = buf_msg(skb);
> + u32 pnet_hash = msg_peer_net_hash(hdr);
>   u16 caps = msg_node_capabilities(hdr);
>   bool legacy = tn->legacy_addr_format;
>   u32 sugg = msg_sugg_node_addr(hdr);
> @@ -242,9 +243,8 @@ void tipc_disc_rcv(struct net *net, struct sk_buff *skb,
>   return;
>   if (!tipc_in_scope(legacy, b->domain, src))
>   return;
> - tipc_node_check_dest(net, src, peer_id, b, caps, signature,
> -  msg_peer_net_hash(hdr), , ,
> -  _addr);
> + tipc_node_check_dest(net, src, peer_id, b, caps, signature, pnet_hash,
> +  , , _addr);
>   if (dupl_addr)
>   disc_dupl_alert(b, src, );
>   if (!respond)
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net-next v3 1/1] tipc: introduce variable window congestion control

[Ying Xue]

>> I don't know you ever remembered many years ago I ever implemented a 
>> prototype to introduce slow-
>> start and traffic congestion avoidance algorithms on link layer :)
>>
>> In my experiment, there were a few of meaningful findings:
>> - It was crucial for the throughput performance about how to set initial 
>> congestion window and upper
>> congestion window size.
>> - It was found that different Ethernet speeds, different CPU capabilities 
>> and different message sizes all have
>> a big impact on throughput performance. I did lots of experiments, as a 
>> result, sometimes performance
>> improvement was very obvious, but sometimes, performance improvement was 
>> minimal even when I
>> measured throughput in a similar network environment. Particularly, if I 
>> slightly changed some test
>> conditions, throughput improvement results were also quite different.
>>
>> At that moment, I ever doubted whether we needed to do the following changes:
>> 1. Should we introduce a timer to measure RTT and identify whether network 
>> congestion happens or not?
> 
> Actually, measuring RTT is one of the stated requirements from the Ericsson 
> product line, and I think we should do it.
> Our plan was to do this by adding an adequately scaled time stamp to 
> probe/probe-reply messages. This in turn could serve as base to calculate the 
> bandwidth product and window limits.
> 

That's great.

>> 2. Should we change message delivery mode from message-oriented to 
>> byte-oriented?
> 
> In my original series, sent out Nov. 4th, I did that in an additional patch. 
> However, the solution was intrusive and ugly, and even gave slightly poorer 
> performance than the current packet base one. So I decided to abandon this 
> approach, at least for now.
> 

Okay.

>>
>> Of course, in my experiment I didn't make so big changes.
>>
>> So I want to know:
>> -  How did you select the minimum window size and maximum window size?
> 
> This was done empirically only. Lower than 50 never made any sense, as 
> throughput always would be lower. 
> Throughput seemed to improve up to 500, but thereafter there was no 
> improvement.

Regarding my previous validation, the initial window size and maximum
parameters are very crucial to performance. Moreover, currently when
receiver's unacked messages reach 16 (TIPC_MIN_LINK_WIN), receiver would
send one acknowledgment through one protocol state message to sender. In
fact TIPC_MIN_LINK_WIN is also very important for performance. If the
value is bigger, it means we can save network bandwidth to send more
data. But if its value is bigger, it means detecting network congestion
or message loss will be less sensitive. In my opinion, we also need to
consider to adjust according test result.

We can discuss more later.

> 
> 
>> -  Did you measure TIPC throughput performance on different Ethernets? 
>> Including large message size test
>> and small message size test.
> 
> I measured it only on virtio. I know this is a weakness, and we should of 
> course try on other drivers as well.
> 
>> - Did you meet similar phenomena to me when we slightly changed test 
>> condition?
> 
> I observed differences, but the overall picture was that this modest change 
> always gave some improvement.
> 
>>
>> In this proposal, during slow-start stage, the window increase is pretty 
>> slow:
>>
>> +if (qlen >= cwin && (l->snd_nxt - buf_seqno(skb_peek(txq)) == qlen)) {
>> +add = l->cong_acks++ % 32 ? 0 : 1;
>> +cwin = min_t(u16, cwin + add, l->max_win);
>> +l->window = cwin;
>> +}
>>
>> But in TCP slow-start algorithm, during slow-start stage congestion window 
>> increase is much more
>> aggressive than above. As long as congestion window exceeds slow-start 
>> threshold, it enters congestion
>> avoidance stage in which congestion window increases slowly.
>>
>> I am curious why the congestion window increase is pretty conservative 
>> compared to TCP slow-start
>> algorithm. What factors did you consider when you selected the algorithm?
> 
> I did it only because it is simple and gives an obvious improvement.  I am 
> fully aware that the growth is very slow, and that introducing a threshold 
> where we go from a faster growth to a slower one probably would increase 
> average window size and throughput.
> I see this as the next step, that we all could experiment with.
> 
> However, my approach now is that this very simple change is a low hanging 
> fruit that gives an obvious improvement, so we lose nothing by releasing i

Re: [tipc-discussion] [PATCH v2 0/5] TIPC encryption

[Ying Xue]

 Acked-by: Ying Xue 

Great work!

Thanks,
Ying

On 11/5/19 6:50 PM, Tuong Lien wrote:
> This series provides TIPC encryption feature, kernel part. There will be
> another one in the 'iproute2/tipc' for user space to set key.
> 
> Tuong Lien (5):
>   tipc: add reference counter to bearer
>   tipc: enable creating a "preliminary" node
>   tipc: add new AEAD key structure for user API
>   tipc: introduce TIPC encryption & authentication
>   tipc: add support for AEAD key setting via netlink
> 
>  include/uapi/linux/tipc.h |   21 +
>  include/uapi/linux/tipc_netlink.h |8 +
>  net/tipc/Kconfig  |   12 +
>  net/tipc/Makefile |1 +
>  net/tipc/bcast.c  |2 +-
>  net/tipc/bearer.c |   54 +-
>  net/tipc/bearer.h |   11 +-
>  net/tipc/core.c   |   18 +
>  net/tipc/core.h   |8 +
>  net/tipc/crypto.c | 1986 
> +
>  net/tipc/crypto.h |  167 
>  net/tipc/link.c   |   19 +-
>  net/tipc/link.h   |1 +
>  net/tipc/msg.c|   15 +-
>  net/tipc/msg.h|   46 +-
>  net/tipc/netlink.c|   20 +-
>  net/tipc/node.c   |  347 ++-
>  net/tipc/node.h   |   13 +
>  net/tipc/sysctl.c |   11 +
>  net/tipc/udp_media.c  |1 +
>  20 files changed, 2694 insertions(+), 67 deletions(-)
>  create mode 100644 net/tipc/crypto.c
>  create mode 100644 net/tipc/crypto.h
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH v3 4/5] tipc: introduce TIPC encryption & authentication

[Ying Xue]

On 11/6/19 6:23 PM, Tuong Lien wrote:
> This commit offers an option to encrypt and authenticate all messaging,
> including the neighbor discovery messages. The currently most advanced
> algorithm supported is the AEAD AES-GCM (like IPSec or TLS). All
> encryption/decryption is done at the bearer layer, just before leaving
> or after entering TIPC.
> 
> Supported features:
> - Encryption & authentication of all TIPC messages (header + data);
> - Two symmetric-key modes: Cluster and Per-node;
> - Automatic key switching;
> - Key-expired revoking (sequence number wrapped);
> - Lock-free encryption/decryption (RCU);
> - Asynchronous crypto, Intel AES-NI supported;
> - Multiple cipher transforms;
> - Logs & statistics;
> 
> Two key modes:
> - Cluster key mode: One single key is used for both TX & RX in all
> nodes in the cluster.
> - Per-node key mode: Each nodes in the cluster has one specific TX key.
> For RX, a node requires its peers' TX key to be able to decrypt the
> messages from those peers.
> 
> Key setting from user-space is performed via netlink by a user program
> (e.g. the iproute2 'tipc' tool).
> 
> Internal key state machine:
> 
>  AttachAlign(RX)
>  +-+   +-+
>  | V   | V
> +-+  Attach +-+
> |  IDLE   |>| PENDING |(user = 0)
> +-+ +-+
>A   A   Switch|  A
>|   | |  |
>|   | Free(switch/revoked)|  |
>  (Free)|   +--+  |  |Timeout
>|  (TX)|  |  |(RX)
>|  |  |  |
>|  |  v  |
> +-+  Switch +-+
> | PASSIVE |<| ACTIVE  |
> +-+   (RX)  +-+
> (user = 1)  (user >= 1)
> 
> The number of TFMs is 10 by default and can be changed via the procfs
> 'net/tipc/max_tfms'. At this moment, as for simplicity, this file is
> also used to print the crypto statistics at runtime:
> 
> echo 0xfff1 > /proc/sys/net/tipc/max_tfms
> 
> The patch defines a new TIPC version (v7) for the encryption message (-
> backward compatibility as well). The message is basically encapsulated
> as follows:
> 
>+--+
>| TIPCv7 encryption  | Original TIPCv2| Authentication |
>| header | packet (encrypted) | Tag|
>+--+
> 
> The throughput is about ~40% for small messages (compared with non-
> encryption) and ~9% for large messages. With the support from hardware
> crypto i.e. the Intel AES-NI CPU instructions, the throughput increases
> upto ~85% for small messages and ~55% for large messages.
> 
> By default, the new feature is inactive (i.e. no encryption) until user
> sets a key for TIPC. There is however also a new option - "TIPC_CRYPTO"
> in the kernel configuration to enable/disable the new code when needed.
> 
> v2: rebase, add new kernel option ("TIPC_CRYPTO")
> v3: remove the "ifdef/else" for the bearer_xmit()
> 
> MAINTAINERS | add two new files 'crypto.h' & 'crypto.c' in tipc
> Signed-off-by: Tuong Lien 

 Acked-by: Ying Xue 

> ---
>  net/tipc/Kconfig |   12 +
>  net/tipc/Makefile|1 +
>  net/tipc/bcast.c |2 +-
>  net/tipc/bearer.c|   35 +-
>  net/tipc/bearer.h|3 +-
>  net/tipc/core.c  |   18 +
>  net/tipc/core.h  |8 +
>  net/tipc/crypto.c| 1986 
> ++
>  net/tipc/crypto.h|  167 +
>  net/tipc/link.c  |   19 +-
>  net/tipc/link.h  |1 +
>  net/tipc/msg.c   |   15 +-
>  net/tipc/msg.h   |   46 +-
>  net/tipc/node.c  |  101 ++-
>  net/tipc/node.h  |8 +
>  net/tipc/sysctl.c|   11 +
>  net/tipc/udp_media.c |1 +
>  17 files changed, 2388 insertions(+), 46 deletions(-)
>  create mode 100644 net/tipc/crypto.c
>  create mode 100644 net/tipc/crypto.h
> 
> diff --git a/net/tipc/Kconfig b/net/tipc/Kconfig
> index b83e16ade4d2..6d94ccf7a9e9 100644
> --- a/net/tipc/Kconfig
> +++ b/net/tipc/Kconfig
> @@ -35,6 +35,18 @@ config TIPC_MEDIA_UDP
> Saying Y here will enable support for running TIPC over IP/UDP
>   bool
>   default y
> +config TIPC_CRYPTO
> + bool "TIPC encryption support"
> + depends on TIPC
> + help
>

Re: [tipc-discussion] [PATCH RFC 0/5] TIPC encryption

[Ying Xue]

On 11/5/19 5:33 AM, Jon Maloy wrote:
> Tuong, Ying
> I am ok with a kernel option, as long as it is enabled by default. I can 
> imagine smaller embedded systems where the deployer want a small module, and 
> encryption anyway is managed differently, or not at all.
> 
> ///jon

When I gave the suggestion to add a new kernel option, I also ever
figured out how to reach this goal based on this series of patches.

In my opinion, we don't need to modify the patches of making some
preparation things. For example, we don't need to change patch #1 at all.

Patch #3, #4 and #5 are almost completely separate with current code.
The only thing is that we need to consider how to modify patch #2. In
sum, it looks like it's not very difficult to introduce the new kernel
configuration option.

The kernel option not only can help us keep TIPC module size smaller,
but also can help us maintain it easily particularly with TIPC becoming
more and more complex. Lastly, probably it can help to make it possible
that the series of patches can be easily accepted by upstream if user
can disable the feature with a kernel option.



___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net-next v3 1/1] tipc: add smart nagle feature

[Ying Xue]

On 10/30/19 4:11 AM, Jon Maloy wrote:
> We introduce a feature that works like a combination of TCP_NAGLE and
> TCP_CORK, but without some of the weaknesses of those. In particular,
> we will not observe long delivery delays because of delayed acks, since
> the algorithm itself decides if and when acks are to be sent from the
> receiving peer.
> 
> - The nagle property as such is determined by manipulating a new
>   'maxnagle' field in struct tipc_sock. If certain conditions are met,
>   'maxnagle' will define max size of the messages which can be bundled.
>   If it is set to zero no messages are ever bundled, implying that the
>   nagle property is disabled.
> - A socket with the nagle property enabled enters nagle mode when more
>   than 4 messages have been sent out without receiving any data message
>   from the peer.
> - A socket leaves nagle mode whenever it receives a data message from
>   the peer.
> 
> In nagle mode, messages smaller than 'maxnagle' are accumulated in the
> socket write queue. The last buffer in the queue is marked with a new
> 'ack_required' bit, which forces the receiving peer to send a CONN_ACK
> message back to the sender upon reception.
> 
> The accumulated contents of the write queue is transmitted when one of
> the following events or conditions occur.
> 
> - A CONN_ACK message is received from the peer.
> - A data message is received from the peer.
> - A SOCK_WAKEUP pseudo message is received from the link level.
> - The write queue contains more than 64 1k blocks of data.
> - The connection is being shut down.
> - There is no CONN_ACK message to expect. I.e., there is currently
>   no outstanding message where the 'ack_required' bit was set. As a
>   consequence, the first message added after we enter nagle mode
>   is always sent directly with this bit set.
> 
> This new feature gives a 50-100% improvement of throughput for small
> (i.e., less than MTU size) messages, while it might add up to one RTT
> to latency time when the socket is in nagle mode.
> 
> Signed-off-by: Jon Maloy 

Acked-by: Ying Xue 

> 
> v2: Added TIPC_NODELAY socket option
> v3: Given TIPC_NODELAY a parameter as suggested by Ying, and simplified
> test for when we can bundle messages.
> ---
>  include/uapi/linux/tipc.h |   1 +
>  net/tipc/msg.c|  53 +
>  net/tipc/msg.h|  12 +
>  net/tipc/node.h   |   7 ++-
>  net/tipc/socket.c | 114 
> +++---
>  5 files changed, 169 insertions(+), 18 deletions(-)
> 
> diff --git a/include/uapi/linux/tipc.h b/include/uapi/linux/tipc.h
> index 7df026e..76421b8 100644
> --- a/include/uapi/linux/tipc.h
> +++ b/include/uapi/linux/tipc.h
> @@ -191,6 +191,7 @@ struct sockaddr_tipc {
>  #define TIPC_GROUP_JOIN 135 /* Takes struct tipc_group_req* */
>  #define TIPC_GROUP_LEAVE136 /* No argument */
>  #define TIPC_SOCK_RECVQ_USED137 /* Default: none (read only) */
> +#define TIPC_NODELAY138 /* Default: false */
>  
>  /*
>   * Flag values
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index 922d262..973795a 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -190,6 +190,59 @@ int tipc_buf_append(struct sk_buff **headbuf, struct 
> sk_buff **buf)
>   return 0;
>  }
>  
> +/**
> + * tipc_msg_append(): Append data to tail of an existing buffer queue
> + * @hdr: header to be used
> + * @m: the data to be appended
> + * @mss: max allowable size of buffer
> + * @dlen: size of data to be appended
> + * @txq: queue to appand to
> + * Returns the number og 1k blocks appended or errno value
> + */
> +int tipc_msg_append(struct tipc_msg *_hdr, struct msghdr *m, int dlen,
> + int mss, struct sk_buff_head *txq)
> +{
> + struct sk_buff *skb, *prev;
> + int accounted, total, curr;
> + int mlen, cpy, rem = dlen;
> + struct tipc_msg *hdr;
> +
> + skb = skb_peek_tail(txq);
> + accounted = skb ? msg_blocks(buf_msg(skb)) : 0;
> + total = accounted;
> +
> + while (rem) {
> + if (!skb || skb->len >= mss) {
> + prev = skb;
> + skb = tipc_buf_acquire(mss, GFP_KERNEL);
> + if (unlikely(!skb))
> + return -ENOMEM;
> + skb_orphan(skb);
> + skb_trim(skb, MIN_H_SIZE);
> + hdr = buf_msg(skb);
> + skb_copy_to_linear_data(skb, _hdr, MIN_H_SIZE);
> + msg_set_hdr_sz(hdr, MIN_H_SIZE);
> + msg_set_size(hdr, MIN_H_SIZE);
> + __skb_qu

Re: [tipc-discussion] [net-next v3 1/1] tipc: add smart nagle feature

[Ying Xue]

On 10/29/19 1:37 AM, Jon Maloy wrote:
> @@ -3007,6 +3068,9 @@ static int tipc_setsockopt(struct socket *sock, int 
> lvl, int opt,
>   case TIPC_GROUP_LEAVE:
>   res = tipc_sk_leave(tsk);
>   break;
> + case TIPC_NODELAY:
> + tsk->nodelay = true;
> + break;
>   default:
>   res = -EINVAL;
>   }

Once user sets tsk->nodelay to true, there is no chance to set it back
to false. Although this scenario rarely happens for us, it's better that
we can provide the function.

For example, below is how TCP supports TCP_NODELAY option:

case TCP_NODELAY:
if (val) {
/* TCP_NODELAY is weaker than TCP_CORK, so that
 * this option on corked socket is remembered, but
 * it is not activated until cork is cleared.
 *
 * However, when TCP_NODELAY is set we make
 * an explicit push, which overrides even TCP_CORK
 * for currently queued segments.
 */
tp->nonagle |= TCP_NAGLE_OFF|TCP_NAGLE_PUSH;
tcp_push_pending_frames(sk);
} else {
tp->nonagle &= ~TCP_NAGLE_OFF;
}
break;


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net-next v2 1/1] tipc: add smart nagle feature

[Ying Xue]

On 10/25/19 12:28 AM, Jon Maloy wrote:
> 1) TIPC_NODELAY might be a good option, although I fear people might misuse 
> it in the belief that TIPC nagle has the same disadvantages as TCP nagle, 
> which it doesn't.
> But ok, I'll add it.
> 
> 2) CONN_PROBE/CONN_PROBE_REPLY are not considered simply because they are so 
> rare (once per hour) that they won't make any difference.
> 
> 3) We don't really have any tools to measure this. The latency measurement in 
> our benchmark tool never trigs nagle mode, so we won't notice any difference.
>  When nagle is enabled, we can only measure latency per direction, not 
> round-trip delay (since there is no return message), but logically it works 
> as follows:
> 
> Scenario 1: 
>  1) Socket goes to nagle mode. The message trigging this is not bundled, 
> but just sent out with the 'response_req' bit set.
>  2) A number of messages and possible skbs are added to the queue.
>  3) The ACK_MSG (response on msg 1) arrives after 1 RTT, and the 
> accumulated messages are sent. So, the first message, probably added just 
> after the 'resp-req' message was sent might have a delay of up to one RTT. 
> The remaining messages in the queue will have a lower delay, and notably a 
> message added just before the ACK_MSG arrives will have almost no delay.
> 
> Scenario 2:
>  1) Socket is in nagle mode, and a number of messages are being 
> accumulated. The last message in the queue always have the resp_req bit set.
>  2) The queue surpasses 64 k, or a larger message than 'maxnagle'is being 
> sent. So the whole send queue is sent out. 
>  3) Obviously we didn't wait for the expected MSG_ACK in this case, so 
> the delay for all messages is less than 1 RTT.
> 
> Remains to know the size of RTT, but in the type of clusters we are running 
> this is rarely more than a few milliseconds, at most. This in contrast to 
> TCP, where the delay may be several hundred milliseconds.
> 

Thank you for your clear explanation. It makes me fully understood why
you stated the delay was no more than one RTT after nagle was enabled.


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [iproute2] tipc: add new commands to set TIPC AEAD key

[Ying Xue]

Tt looks like we will use "tipc node" command to configure static key to
TIPC module, right?

Do we plan to support dynamic key setting? If yes, what kinds of key
exchange protocol would we use? For example, in IPSEC, it uses IKEv2 as
its key exchange protocol.

Will key be expired after a specific lifetime? For instance, in
IPSEC/Raccoon2 or strongswan, they use rekey feature to provide this
function to make security association safer.

On 10/14/19 7:36 PM, Tuong Lien wrote:
> Two new commands are added as part of 'tipc node' command:
> 
>  $tipc node set key KEY [algname ALGNAME] [nodeid NODEID]
>  $tipc node flush key
> 
> which enable user to set and remove AEAD keys in kernel TIPC.
> 
> For the 'set key' command, the given 'nodeid' parameter decides the
> mode to be applied to the key, particularly:
> 
> - If NODEID is empty, the key is a 'cluster' key which will be used for
> all message encryption/decryption from/to the node (i.e. both TX & RX).
> The same key needs to be set in the other nodes i.e. the 'cluster key'
> mode.
> 
> - If NODEID is own node, the key is used for message encryption (TX)
> from the node. Whereas, if NODEID is a peer node, the key is for
> message decryption (RX) from that peer node.
> This is the 'per-node-key' mode that each nodes in the cluster has its
> specific (TX) key.
> 
> Signed-off-by: Tuong Lien 
> ---
>  include/uapi/linux/tipc.h |  21 ++
>  include/uapi/linux/tipc_netlink.h |   4 ++
>  tipc/misc.c   |  38 +++
>  tipc/misc.h   |   1 +
>  tipc/node.c   | 133 
> +-
>  5 files changed, 195 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/tipc.h b/include/uapi/linux/tipc.h
> index e16cb4e2..b118ce9b 100644
> --- a/include/uapi/linux/tipc.h
> +++ b/include/uapi/linux/tipc.h
> @@ -232,6 +232,27 @@ struct tipc_sioc_nodeid_req {
>   char node_id[TIPC_NODEID_LEN];
>  };
>  
> +/*
> + * TIPC Crypto, AEAD mode
> + */
> +#define TIPC_AEAD_MAX_ALG_NAME   (32)
> +#define TIPC_AEAD_MIN_KEYLEN (16 + 4)
> +#define TIPC_AEAD_MAX_KEYLEN (32 + 4)
> +
> +struct tipc_aead_key {
> + char alg_name[TIPC_AEAD_MAX_ALG_NAME];
> + unsigned int keylen;/* in bytes */
> + char key[];
> +};
> +
> +#define TIPC_AEAD_KEY_MAX_SIZE   (sizeof(struct tipc_aead_key) + \
> + TIPC_AEAD_MAX_KEYLEN)
> +
> +static inline int tipc_aead_key_size(struct tipc_aead_key *key)
> +{
> + return sizeof(*key) + key->keylen;
> +}
> +
>  /* The macros and functions below are deprecated:
>   */
>  
> diff --git a/include/uapi/linux/tipc_netlink.h 
> b/include/uapi/linux/tipc_netlink.h
> index efb958fd..6c2194ab 100644
> --- a/include/uapi/linux/tipc_netlink.h
> +++ b/include/uapi/linux/tipc_netlink.h
> @@ -63,6 +63,8 @@ enum {
>   TIPC_NL_PEER_REMOVE,
>   TIPC_NL_BEARER_ADD,
>   TIPC_NL_UDP_GET_REMOTEIP,
> + TIPC_NL_KEY_SET,
> + TIPC_NL_KEY_FLUSH,
>  
>   __TIPC_NL_CMD_MAX,
>   TIPC_NL_CMD_MAX = __TIPC_NL_CMD_MAX - 1
> @@ -160,6 +162,8 @@ enum {
>   TIPC_NLA_NODE_UNSPEC,
>   TIPC_NLA_NODE_ADDR, /* u32 */
>   TIPC_NLA_NODE_UP,   /* flag */
> + TIPC_NLA_NODE_ID,   /* data */
> + TIPC_NLA_NODE_KEY,  /* data */
>  
>   __TIPC_NLA_NODE_MAX,
>   TIPC_NLA_NODE_MAX = __TIPC_NLA_NODE_MAX - 1
> diff --git a/tipc/misc.c b/tipc/misc.c
> index e4b1cd0c..1daf3072 100644
> --- a/tipc/misc.c
> +++ b/tipc/misc.c
> @@ -98,6 +98,44 @@ int str2nodeid(char *str, uint8_t *id)
>   return 0;
>  }
>  
> +int str2key(char *str, struct tipc_aead_key *key)
> +{
> + int len = strlen(str);
> + int ishex = 0;
> + int i;
> +
> + /* Check if the input is a hex string (i.e. 0x...) */
> + if (len > 2 && strncmp(str, "0x", 2) == 0) {
> + ishex = is_hex(str + 2, len - 2 - 1);
> + if (ishex) {
> + len -= 2;
> + str += 2;
> + }
> + }
> +
> + /* Obtain key: */
> + if (!ishex) {
> + key->keylen = len;
> + memcpy(key->key, str, len);
> + } else {
> + /* Convert hex string to key */
> + key->keylen = (len + 1) / 2;
> + for (i = 0; i < key->keylen; i++) {
> + if (i == 0 && len % 2 != 0) {
> + if (sscanf(str, "%1hhx", >key[0]) != 1)
> + return -1;
> + str += 1;
> + continue;
> + }
> + if (sscanf(str, "%2hhx", >key[i]) != 1)
> + return -1;
> + str += 2;
> + }
> + }
> +
> + return 0;
> +}
> +
>  void nodeid2str(uint8_t *id, char *str)
>  {
>   int i;
> diff --git a/tipc/misc.h b/tipc/misc.h
> index ff2f31f1..59309f68 100644
> --- a/tipc/misc.h
> +++ b/tipc/misc.h
> 

Re: [tipc-discussion] [PATCH RFC 0/5] TIPC encryption

[Ying Xue]

Looks like this is an amazing proposal!

I had the idea long time ago, but at that moment, I didn't think
encrypting TIPC message was meaningful because TIPC was mostly used
within internal network. After UDP bearer was supported and one TIPC
node was capable of communicating with its peers across IP, it seemed
the encryption feature became useful. But if needed, we could enable
IPSEC during this situation.

At present, the only useful scenario that I can image is that TIPC will
be used as low level communication infrastructure in Docker or k8s
environment. Is there other case?

Sorry, I am pretty busy in this week, and significant changes are made
in the series. I have to take a bit long time to review the series.
Please wait for a while.

On 10/14/19 7:07 PM, Tuong Lien wrote:
> This series provides TIPC encryption feature, kernel part. There will be
> another one in the 'iproute2/tipc' for user space to set key.
> 
> Tuong Lien (5):
>   tipc: add reference counter to bearer
>   tipc: enable creating a "preliminary" node
>   tipc: add new AEAD key structure for user API
>   tipc: introduce TIPC encryption & authentication
>   tipc: add support for AEAD key setting via netlink
> 
>  include/uapi/linux/tipc.h |   21 +
>  include/uapi/linux/tipc_netlink.h |4 +
>  net/tipc/Makefile |2 +-
>  net/tipc/bcast.c  |2 +-
>  net/tipc/bearer.c |   52 +-
>  net/tipc/bearer.h |6 +-
>  net/tipc/core.c   |   10 +
>  net/tipc/core.h   |4 +
>  net/tipc/crypto.c | 1986 
> +
>  net/tipc/crypto.h |  166 
>  net/tipc/link.c   |   16 +-
>  net/tipc/link.h   |1 +
>  net/tipc/msg.c|   24 +-
>  net/tipc/msg.h|   44 +-
>  net/tipc/netlink.c|   16 +-
>  net/tipc/node.c   |  314 +-
>  net/tipc/node.h   |   10 +
>  net/tipc/sysctl.c |9 +
>  net/tipc/udp_media.c  |1 +
>  19 files changed, 2604 insertions(+), 84 deletions(-)
>  create mode 100644 net/tipc/crypto.c
>  create mode 100644 net/tipc/crypto.h
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net-next] tipc: improve throughput between nodes in netns

[Ying Xue]

On 10/15/19 7:46 PM, Jon Maloy wrote:
> You must have forgot that since commit 6c9081a3915d ("add loopback device 
> tracing") this is no problem any more.
> Of course we do the same in this case, so a trouble shooter only needs to do 
> tcpdump on the sender's loopback interface.

Oh, the most inconvenience is gone. Please move on.


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] TIPC Contianer

[Ying Xue]

On 10/16/19 4:41 AM, Mohamed Hamed El-Gamal wrote:
> Hello,
> 
> I would like to ask you regarding the optimum way to run TIPC over
> containerized workload
> Will it be using MACVLAN interfaces for docker and K8s ?
> 

Interfaces attached to different containers are connected to a virtual
bridge on host, and each of contains serves as TIPC node, which
definitely works.

I think macvlan interface used as TIPC bearer for docker or k8s should
be the most efficient way. But particularly when creating macvlan
interface, it should use "bridge" mode, otherwise, TIPC links between
different containers cannot be established.

But I don't do any experiment to verify whether it works to configure
maclvan interface as TIPC bearer.

If you would like to try, please tell your test result.

> Note: We are using also Mulit-netowkring
> 
> 
> It would be great if there is any supportive documentations
> 
> 
> Thanks a lot
> Best Regards
> 
> 
> 
> 
> 
> Garanti
> sans virus. www.avast.com
> 
> <#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>
> 
> ___
> tipc-discussion mailing list
> tipc-discussion@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net v1 2/3] tipc: fix wrong socket reference counter after tipc_sk_timeout() returns

[Ying Xue]

On 8/13/19 6:01 PM, Tung Nguyen wrote:
> When tipc_sk_timeout() is executed but user space is grabbing
> ownership, this function rearms itself and returns. However, the
> socket reference counter is not reduced. This causes potential
> unexpected behavior.
> 
> This commit fixes it by calling sock_put() before tipc_sk_timeout()
> returns in the above-mentioned case.
> 
> Fixes: afe8792fec69 ("tipc: refactor function tipc_sk_timeout()")
> Signed-off-by: Tung Nguyen 

Acked-by: Ying Xue 

> ---
>  net/tipc/socket.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index dcb8b6082757..9fd9a5727786 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -2683,6 +2683,7 @@ static void tipc_sk_timeout(struct timer_list *t)
>   if (sock_owned_by_user(sk)) {
>   sk_reset_timer(sk, >sk_timer, jiffies + HZ / 20);
>   bh_unlock_sock(sk);
> + sock_put(sk);
>   return;
>   }
>  
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net v1 1/3] tipc: fix potential memory leak in __tipc_sendmsg()

[Ying Xue]

On 8/13/19 6:01 PM, Tung Nguyen wrote:
> When initiating a connection message to a server side, the connection
> message is cloned and added to the socket write queue. However, if the
> cloning is failed, only the socket write queue is purged. It causes
> memory leak because the original connection message is not freed.
> 
> This commit fixes it by purging the list of connection message when
> it cannot be cloned.
> 
> Fixes: 6787927475e5 ("tipc: buffer overflow handling in listener socket")
> Reported-by: Hoang Le 
> Signed-off-by: Tung Nguyen 

Acked-by: Ying Xue 

> ---
>  net/tipc/socket.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index 83ae41d7e554..dcb8b6082757 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -1392,8 +1392,10 @@ static int __tipc_sendmsg(struct socket *sock, struct 
> msghdr *m, size_t dlen)
>   rc = tipc_msg_build(hdr, m, 0, dlen, mtu, );
>   if (unlikely(rc != dlen))
>   return rc;
> - if (unlikely(syn && !tipc_msg_skb_clone(, >sk_write_queue)))
> + if (unlikely(syn && !tipc_msg_skb_clone(, >sk_write_queue))) {
> + __skb_queue_purge();
>   return -ENOMEM;
> + }
>  
>   trace_tipc_sk_sendmsg(sk, skb_peek(), TIPC_DUMP_SK_SNDQ, " ");
>   rc = tipc_node_xmit(net, , dnode, tsk->portid);
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [PATCH v2 1/3] tipc: fix memory leak issue

[Ying Xue]

syzbot found the following memory leak:

[   68.602482][ T7130] kmemleak: 2 new suspected memory leaks (see 
/sys/kernel/debug/kmemleak)
BUG: memory leak
unreferenced object 0x88810df83c00 (size 512):
  comm "softirq", pid 0, jiffies 4294942354 (age 19.830s)
  hex dump (first 32 bytes):
38 1a 0d 0f 81 88 ff ff 38 1a 0d 0f 81 88 ff ff  8...8...
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
  backtrace:
[<9375ee42>] kmem_cache_alloc_node+0x153/0x2a0
[<4c563922>] __alloc_skb+0x6e/0x210
[<ec87bfa1>] tipc_buf_acquire+0x2f/0x80
[<d151ef84>] tipc_msg_create+0x37/0xe0
[<8bb437b0>] tipc_group_create_event+0xb3/0x1b0
[<947b1d0f>] tipc_group_proto_rcv+0x569/0x640
[<b75ab039>] tipc_sk_filter_rcv+0x9ac/0xf20
[<0dab7a6c>] tipc_sk_rcv+0x494/0x8a0
[<023a7ddd>] tipc_node_xmit+0x196/0x1f0
[<337dd9eb>] tipc_node_distr_xmit+0x7d/0x120
[<b6375182>] tipc_group_delete+0xe6/0x130
[<0361ba2b>] tipc_sk_leave+0x57/0xb0
[<9df90505>] tipc_release+0x7b/0x5e0
[<9f3189da>] __sock_release+0x4b/0xe0
[<d3568ee0>] sock_close+0x1b/0x30
[<266a6215>] __fput+0xed/0x300

Reported-by: syzbot+78fbe679c8ca8d264...@syzkaller.appspotmail.com
Signed-off-by: Hillf Danton 
Signed-off-by: Ying Xue 
---
 net/tipc/node.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/tipc/node.c b/net/tipc/node.c
index 7ca0190..d1852fc 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -1469,10 +1469,13 @@ int tipc_node_xmit(struct net *net, struct sk_buff_head 
*list,
spin_unlock_bh(>lock);
tipc_node_read_unlock(n);
 
-   if (unlikely(rc == -ENOBUFS))
+   if (unlikely(rc == -ENOBUFS)) {
tipc_node_link_down(n, bearer_id, false);
-   else
+   skb_queue_purge(list);
+   skb_queue_purge();
+   } else {
tipc_bearer_xmit(net, bearer_id, , >maddr);
+   }
 
tipc_node_put(n);
 
-- 
2.7.4



___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [PATCH v2 3/3] tipc: fix issue of calling smp_processor_id() in preemptible

[Ying Xue]

2] RDX:  RSI: 2580 RDI: 
0003
[   81.582152][ T8612] RBP: 006cf018 R08: 0001 R09: 
004002e0
[   81.590113][ T8612] R10: 0008 R11: 0246 R12: 
00402320
[   81.598089][ T8612] R13: 004023b0 R14:  R15: 
00

In commit e9c1a793210f ("tipc: add dst_cache support for udp media")
dst_cache_get() was introduced to be called in tipc_udp_xmit(). But
smp_processor_id() called by dst_cache_get() cannot be invoked in
preemptible context, as a result, the complaint above was reported.

Fixes: e9c1a793210f ("tipc: add dst_cache support for udp media")
Reported-by: syzbot+1a68504d96cd17b33...@syzkaller.appspotmail.com
Signed-off-by: Hillf Danton 
Signed-off-by: Ying Xue 
---
 net/tipc/udp_media.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
index 287df687..ca3ae2e 100644
--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c
@@ -224,6 +224,8 @@ static int tipc_udp_send_msg(struct net *net, struct 
sk_buff *skb,
struct udp_bearer *ub;
int err = 0;
 
+   local_bh_disable();
+
if (skb_headroom(skb) < UDP_MIN_HEADROOM) {
err = pskb_expand_head(skb, UDP_MIN_HEADROOM, 0, GFP_ATOMIC);
if (err)
@@ -237,9 +239,12 @@ static int tipc_udp_send_msg(struct net *net, struct 
sk_buff *skb,
goto out;
}
 
-   if (addr->broadcast != TIPC_REPLICAST_SUPPORT)
-   return tipc_udp_xmit(net, skb, ub, src, dst,
->rcast.dst_cache);
+   if (addr->broadcast != TIPC_REPLICAST_SUPPORT) {
+   err = tipc_udp_xmit(net, skb, ub, src, dst,
+   >rcast.dst_cache);
+   local_bh_enable();
+   return err;
+   }
 
/* Replicast, send an skb to each configured IP address */
list_for_each_entry_rcu(rcast, >rcast.list, list) {
@@ -259,6 +264,7 @@ static int tipc_udp_send_msg(struct net *net, struct 
sk_buff *skb,
err = 0;
 out:
kfree_skb(skb);
+   local_bh_enable();
return err;
 }
 
-- 
2.7.4



___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [PATCH v2 2/3] tipc: fix memory leak issue

[Ying Xue]

syzbot found the following memory leak issue:

[   72.286706][ T7064] kmemleak: 1 new suspected memory leaks (see 
/sys/kernel/debug/kmemleak)
BUG: memory leak
unreferenced object 0x888122bca200 (size 128):
  comm "syz-executor232", pid 7065, jiffies 4294943817 (age 8.880s)
  hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
00 00 00 00 00 00 00 00 18 a2 bc 22 81 88 ff ff  ..."
  backtrace:
[<5bada299>] kmem_cache_alloc_trace+0x145/0x2c0
[<e7bcdc9f>] tipc_group_create_member+0x3c/0x190
[<05f56f40>] tipc_group_add_member+0x34/0x40
[<44406683>] tipc_nametbl_build_group+0x9b/0xf0
[<9f71e803>] tipc_setsockopt+0x170/0x490
[<7f61cbc2>] __sys_setsockopt+0x10f/0x220
[<cc630372>] __x64_sys_setsockopt+0x26/0x30
[<ec30be33>] do_syscall_64+0x76/0x1a0
[<271be3e6>] entry_SYSCALL_64_after_hwframe+0x44/0xa9

Reported-by: syzbot+f95d90c454864b3b5...@syzkaller.appspotmail.com
Signed-off-by: Hillf Danton 
Signed-off-by: Ying Xue 
---
 net/tipc/group.c | 22 +-
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/net/tipc/group.c b/net/tipc/group.c
index 5f98d38..cbc540a 100644
--- a/net/tipc/group.c
+++ b/net/tipc/group.c
@@ -273,8 +273,8 @@ static struct tipc_member *tipc_group_find_node(struct 
tipc_group *grp,
return NULL;
 }
 
-static void tipc_group_add_to_tree(struct tipc_group *grp,
-  struct tipc_member *m)
+struct tipc_member *tipc_group_add_to_tree(struct tipc_group *grp,
+  struct tipc_member *m)
 {
u64 nkey, key = (u64)m->node << 32 | m->port;
struct rb_node **n, *parent = NULL;
@@ -282,7 +282,6 @@ static void tipc_group_add_to_tree(struct tipc_group *grp,
 
n = >members.rb_node;
while (*n) {
-   tmp = container_of(*n, struct tipc_member, tree_node);
parent = *n;
tmp = container_of(parent, struct tipc_member, tree_node);
nkey = (u64)tmp->node << 32 | tmp->port;
@@ -291,17 +290,18 @@ static void tipc_group_add_to_tree(struct tipc_group *grp,
else if (key > nkey)
n = &(*n)->rb_right;
else
-   return;
+   return tmp;
}
rb_link_node(>tree_node, parent, n);
rb_insert_color(>tree_node, >members);
+   return m;
 }
 
 static struct tipc_member *tipc_group_create_member(struct tipc_group *grp,
u32 node, u32 port,
u32 instance, int state)
 {
-   struct tipc_member *m;
+   struct tipc_member *m, *n;
 
m = kzalloc(sizeof(*m), GFP_ATOMIC);
if (!m)
@@ -315,10 +315,14 @@ static struct tipc_member 
*tipc_group_create_member(struct tipc_group *grp,
m->instance = instance;
m->bc_acked = grp->bc_snd_nxt - 1;
grp->member_cnt++;
-   tipc_group_add_to_tree(grp, m);
-   tipc_nlist_add(>dests, m->node);
-   m->state = state;
-   return m;
+   n = tipc_group_add_to_tree(grp, m);
+   if (n == m) {
+   tipc_nlist_add(>dests, m->node);
+   m->state = state;
+   } else {
+   kfree(m);
+   }
+   return n;
 }
 
 void tipc_group_add_member(struct tipc_group *grp, u32 node,
-- 
2.7.4



___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [PATCH 1/3] tipc: fix memory leak issue

[Ying Xue]

syzbot found the following memory leak:

[   68.602482][ T7130] kmemleak: 2 new suspected memory leaks (see 
/sys/kernel/debug/kmemleak)
BUG: memory leak
unreferenced object 0x88810df83c00 (size 512):
  comm "softirq", pid 0, jiffies 4294942354 (age 19.830s)
  hex dump (first 32 bytes):
38 1a 0d 0f 81 88 ff ff 38 1a 0d 0f 81 88 ff ff  8...8...
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
  backtrace:
[<9375ee42>] kmem_cache_alloc_node+0x153/0x2a0
[<4c563922>] __alloc_skb+0x6e/0x210
[<ec87bfa1>] tipc_buf_acquire+0x2f/0x80
[<d151ef84>] tipc_msg_create+0x37/0xe0
[<8bb437b0>] tipc_group_create_event+0xb3/0x1b0
[<947b1d0f>] tipc_group_proto_rcv+0x569/0x640
[<b75ab039>] tipc_sk_filter_rcv+0x9ac/0xf20
[<0dab7a6c>] tipc_sk_rcv+0x494/0x8a0
[<023a7ddd>] tipc_node_xmit+0x196/0x1f0
[<337dd9eb>] tipc_node_distr_xmit+0x7d/0x120
[<b6375182>] tipc_group_delete+0xe6/0x130
[<0361ba2b>] tipc_sk_leave+0x57/0xb0
[<9df90505>] tipc_release+0x7b/0x5e0
[<9f3189da>] __sock_release+0x4b/0xe0
[<d3568ee0>] sock_close+0x1b/0x30
[<266a6215>] __fput+0xed/0x300

Reported-by: syzbot+78fbe679c8ca8d264...@syzkaller.appspotmail.com
Signed-off-by: Hillf Danton 
Signed-off-by: Ying Xue 
---
 net/tipc/node.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/tipc/node.c b/net/tipc/node.c
index 7ca0190..d1852fc 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -1469,10 +1469,13 @@ int tipc_node_xmit(struct net *net, struct sk_buff_head 
*list,
spin_unlock_bh(>lock);
tipc_node_read_unlock(n);
 
-   if (unlikely(rc == -ENOBUFS))
+   if (unlikely(rc == -ENOBUFS)) {
tipc_node_link_down(n, bearer_id, false);
-   else
+   skb_queue_purge(list);
+   skb_queue_purge();
+   } else {
tipc_bearer_xmit(net, bearer_id, , >maddr);
+   }
 
tipc_node_put(n);
 
-- 
2.7.4



___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [PATCH 0/3] Fix three issues found by syzbot

[Ying Xue]

In this series, try to fix two memory leak issues and another issue of
calling smp_processor_id() in preemptible context.

Ying Xue (3):
  tipc: fix memory leak issue
  tipc: fix memory leak issue
  tipc: fix issue of calling smp_processor_id() in preemptible

 net/tipc/group.c | 22 +-
 net/tipc/node.c  |  7 +--
 net/tipc/udp_media.c | 12 +---
 3 files changed, 27 insertions(+), 14 deletions(-)

-- 
2.7.4



___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [PATCH 2/3] tipc: fix memory leak issue

[Ying Xue]

syzbot found the following memory leak issue:

[   72.286706][ T7064] kmemleak: 1 new suspected memory leaks (see 
/sys/kernel/debug/kmemleak)
BUG: memory leak
unreferenced object 0x888122bca200 (size 128):
  comm "syz-executor232", pid 7065, jiffies 4294943817 (age 8.880s)
  hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
00 00 00 00 00 00 00 00 18 a2 bc 22 81 88 ff ff  ..."
  backtrace:
[<5bada299>] kmem_cache_alloc_trace+0x145/0x2c0
[<e7bcdc9f>] tipc_group_create_member+0x3c/0x190
[<05f56f40>] tipc_group_add_member+0x34/0x40
[<44406683>] tipc_nametbl_build_group+0x9b/0xf0
[<9f71e803>] tipc_setsockopt+0x170/0x490
[<7f61cbc2>] __sys_setsockopt+0x10f/0x220
[<cc630372>] __x64_sys_setsockopt+0x26/0x30
[<ec30be33>] do_syscall_64+0x76/0x1a0
[<271be3e6>] entry_SYSCALL_64_after_hwframe+0x44/0xa9

Reported-by: syzbot+f95d90c454864b3b5...@syzkaller.appspotmail.com
Signed-off-by: Hillf Danton 
Signed-off-by: Ying Xue 
---
 net/tipc/group.c | 22 +-
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/net/tipc/group.c b/net/tipc/group.c
index 5f98d38..cbc540a 100644
--- a/net/tipc/group.c
+++ b/net/tipc/group.c
@@ -273,8 +273,8 @@ static struct tipc_member *tipc_group_find_node(struct 
tipc_group *grp,
return NULL;
 }
 
-static void tipc_group_add_to_tree(struct tipc_group *grp,
-  struct tipc_member *m)
+struct tipc_member *tipc_group_add_to_tree(struct tipc_group *grp,
+  struct tipc_member *m)
 {
u64 nkey, key = (u64)m->node << 32 | m->port;
struct rb_node **n, *parent = NULL;
@@ -282,7 +282,6 @@ static void tipc_group_add_to_tree(struct tipc_group *grp,
 
n = >members.rb_node;
while (*n) {
-   tmp = container_of(*n, struct tipc_member, tree_node);
parent = *n;
tmp = container_of(parent, struct tipc_member, tree_node);
nkey = (u64)tmp->node << 32 | tmp->port;
@@ -291,17 +290,18 @@ static void tipc_group_add_to_tree(struct tipc_group *grp,
else if (key > nkey)
n = &(*n)->rb_right;
else
-   return;
+   return tmp;
}
rb_link_node(>tree_node, parent, n);
rb_insert_color(>tree_node, >members);
+   return m;
 }
 
 static struct tipc_member *tipc_group_create_member(struct tipc_group *grp,
u32 node, u32 port,
u32 instance, int state)
 {
-   struct tipc_member *m;
+   struct tipc_member *m, *n;
 
m = kzalloc(sizeof(*m), GFP_ATOMIC);
if (!m)
@@ -315,10 +315,14 @@ static struct tipc_member 
*tipc_group_create_member(struct tipc_group *grp,
m->instance = instance;
m->bc_acked = grp->bc_snd_nxt - 1;
grp->member_cnt++;
-   tipc_group_add_to_tree(grp, m);
-   tipc_nlist_add(>dests, m->node);
-   m->state = state;
-   return m;
+   n = tipc_group_add_to_tree(grp, m);
+   if (n == m) {
+   tipc_nlist_add(>dests, m->node);
+   m->state = state;
+   } else {
+   kfree(m);
+   }
+   return n;
 }
 
 void tipc_group_add_member(struct tipc_group *grp, u32 node,
-- 
2.7.4



___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [PATCH 3/3] tipc: fix issue of calling smp_processor_id() in preemptible

[Ying Xue]

2] RDX:  RSI: 2580 RDI: 
0003
[   81.582152][ T8612] RBP: 006cf018 R08: 0001 R09: 
004002e0
[   81.590113][ T8612] R10: 0008 R11: 0246 R12: 
00402320
[   81.598089][ T8612] R13: 004023b0 R14:  R15: 
00

In commit e9c1a793210f ("tipc: add dst_cache support for udp media")
dst_cache_get() was introduced to be called in tipc_udp_xmit(). But
smp_processor_id() called by dst_cache_get() cannot be invoked in
preemptible context, as a result, the complaint above was reported.

Fixes: e9c1a793210f ("tipc: add dst_cache support for udp media")
syzbot+1a68504d96cd17b33...@syzkaller.appspotmail.com
Signed-off-by: Hillf Danton 
Signed-off-by: Ying Xue 
---
 net/tipc/udp_media.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
index 287df687..ca3ae2e 100644
--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c
@@ -224,6 +224,8 @@ static int tipc_udp_send_msg(struct net *net, struct 
sk_buff *skb,
struct udp_bearer *ub;
int err = 0;
 
+   local_bh_disable();
+
if (skb_headroom(skb) < UDP_MIN_HEADROOM) {
err = pskb_expand_head(skb, UDP_MIN_HEADROOM, 0, GFP_ATOMIC);
if (err)
@@ -237,9 +239,12 @@ static int tipc_udp_send_msg(struct net *net, struct 
sk_buff *skb,
goto out;
}
 
-   if (addr->broadcast != TIPC_REPLICAST_SUPPORT)
-   return tipc_udp_xmit(net, skb, ub, src, dst,
->rcast.dst_cache);
+   if (addr->broadcast != TIPC_REPLICAST_SUPPORT) {
+   err = tipc_udp_xmit(net, skb, ub, src, dst,
+   >rcast.dst_cache);
+   local_bh_enable();
+   return err;
+   }
 
/* Replicast, send an skb to each configured IP address */
list_for_each_entry_rcu(rcast, >rcast.list, list) {
@@ -259,6 +264,7 @@ static int tipc_udp_send_msg(struct net *net, struct 
sk_buff *skb,
err = 0;
 out:
kfree_skb(skb);
+   local_bh_enable();
return err;
 }
 
-- 
2.7.4



___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [PATCH 1/3] tipc: fix memory leak issue

[Ying Xue]

syzbot found the following memory leak:

[   68.602482][ T7130] kmemleak: 2 new suspected memory leaks (see 
/sys/kernel/debug/kmemleak)
BUG: memory leak
unreferenced object 0x88810df83c00 (size 512):
  comm "softirq", pid 0, jiffies 4294942354 (age 19.830s)
  hex dump (first 32 bytes):
38 1a 0d 0f 81 88 ff ff 38 1a 0d 0f 81 88 ff ff  8...8...
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
  backtrace:
[<9375ee42>] kmem_cache_alloc_node+0x153/0x2a0
[<4c563922>] __alloc_skb+0x6e/0x210
[<ec87bfa1>] tipc_buf_acquire+0x2f/0x80
[<d151ef84>] tipc_msg_create+0x37/0xe0
[<8bb437b0>] tipc_group_create_event+0xb3/0x1b0
[<947b1d0f>] tipc_group_proto_rcv+0x569/0x640
[<b75ab039>] tipc_sk_filter_rcv+0x9ac/0xf20
[<0dab7a6c>] tipc_sk_rcv+0x494/0x8a0
[<023a7ddd>] tipc_node_xmit+0x196/0x1f0
[<337dd9eb>] tipc_node_distr_xmit+0x7d/0x120
[<b6375182>] tipc_group_delete+0xe6/0x130
[<0361ba2b>] tipc_sk_leave+0x57/0xb0
[<9df90505>] tipc_release+0x7b/0x5e0
[<9f3189da>] __sock_release+0x4b/0xe0
[<d3568ee0>] sock_close+0x1b/0x30
[<266a6215>] __fput+0xed/0x300

Reported-by: syzbot+78fbe679c8ca8d264...@syzkaller.appspotmail.com
Signed-off-by: Hillf Danton 
Signed-off-by: Ying Xue 
---
 net/tipc/node.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/tipc/node.c b/net/tipc/node.c
index 7ca0190..d1852fc 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -1469,10 +1469,13 @@ int tipc_node_xmit(struct net *net, struct sk_buff_head 
*list,
spin_unlock_bh(>lock);
tipc_node_read_unlock(n);
 
-   if (unlikely(rc == -ENOBUFS))
+   if (unlikely(rc == -ENOBUFS)) {
tipc_node_link_down(n, bearer_id, false);
-   else
+   skb_queue_purge(list);
+   skb_queue_purge();
+   } else {
tipc_bearer_xmit(net, bearer_id, , >maddr);
+   }
 
tipc_node_put(n);
 
-- 
2.7.4



___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [PATCH 3/3] tipc: fix issue of calling smp_processor_id() in preemptible

[Ying Xue]

2] RDX:  RSI: 2580 RDI: 
0003
[   81.582152][ T8612] RBP: 006cf018 R08: 0001 R09: 
004002e0
[   81.590113][ T8612] R10: 0008 R11: 0246 R12: 
00402320
[   81.598089][ T8612] R13: 004023b0 R14:  R15: 
00

In commit e9c1a793210f ("tipc: add dst_cache support for udp media")
dst_cache_get() was introduced to be called in tipc_udp_xmit(). But
smp_processor_id() called by dst_cache_get() cannot be invoked in
preemptible context, as a result, the complaint above was reported.

Fixes: e9c1a793210f ("tipc: add dst_cache support for udp media")
syzbot+1a68504d96cd17b33...@syzkaller.appspotmail.com
Signed-off-by: Hillf Danton 
Signed-off-by: Ying Xue 
---
 net/tipc/udp_media.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
index 287df687..ca3ae2e 100644
--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c
@@ -224,6 +224,8 @@ static int tipc_udp_send_msg(struct net *net, struct 
sk_buff *skb,
struct udp_bearer *ub;
int err = 0;
 
+   local_bh_disable();
+
if (skb_headroom(skb) < UDP_MIN_HEADROOM) {
err = pskb_expand_head(skb, UDP_MIN_HEADROOM, 0, GFP_ATOMIC);
if (err)
@@ -237,9 +239,12 @@ static int tipc_udp_send_msg(struct net *net, struct 
sk_buff *skb,
goto out;
}
 
-   if (addr->broadcast != TIPC_REPLICAST_SUPPORT)
-   return tipc_udp_xmit(net, skb, ub, src, dst,
->rcast.dst_cache);
+   if (addr->broadcast != TIPC_REPLICAST_SUPPORT) {
+   err = tipc_udp_xmit(net, skb, ub, src, dst,
+   >rcast.dst_cache);
+   local_bh_enable();
+   return err;
+   }
 
/* Replicast, send an skb to each configured IP address */
list_for_each_entry_rcu(rcast, >rcast.list, list) {
@@ -259,6 +264,7 @@ static int tipc_udp_send_msg(struct net *net, struct 
sk_buff *skb,
err = 0;
 out:
kfree_skb(skb);
+   local_bh_enable();
return err;
 }
 
-- 
2.7.4



___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [PATCH 0/3] Fix three issues found by syzbot

[Ying Xue]

In this series, try to fix two memory leak issues and another issue of
calling smp_processor_id() in preemptible context.

Ying Xue (3):
  tipc: fix memory leak issue
  tipc: fix memory leak issue
  tipc: fix issue of calling smp_processor_id() in preemptible

 net/tipc/group.c | 22 +-
 net/tipc/node.c  |  7 +--
 net/tipc/udp_media.c | 12 +---
 3 files changed, 27 insertions(+), 14 deletions(-)

-- 
2.7.4



___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net] tipc: fix false detection of retransmit failures

[Ying Xue]

Hi Tuong,

Thank you for your clear explanation.

I am fine to this patch. Good work!

Regards,
Ying


On 8/1/19 10:58 AM, Tuong Lien Tong wrote:
> Hi Ying,
> 
> See below my answers inline.
> 
> BR/Tuong
> 
> -Original Message-
> From: Ying Xue  
> Sent: Wednesday, July 31, 2019 8:21 PM
> To: Tuong Lien ; 
> tipc-discussion@lists.sourceforge.net; jon.ma...@ericsson.com; 
> ma...@donjonn.com
> Subject: Re: [net] tipc: fix false detection of retransmit failures
> 
> On 7/19/19 11:56 AM, Tuong Lien wrote:
>> This commit eliminates the use of the link 'stale_limit' & 'prev_from'
>> (besides the already removed - 'stale_cnt') variables in the detection
>> of repeated retransmit failures as there is no proper way to initialize
>> them to avoid a false detection, i.e. it is not really a retransmission
>> failure but due to a garbage values in the variables.
> 
> Sorry, I couldn't understand the reason why we have no proper way to
> initialize 'stale_limit' & 'prev_from' variables of tipc_link struct.
> 
> As far as I understand, the two variables have been set to zero when
> tipc_link object is allocated with kzalloc() in tipc_link_create().
> 
> Can you please help me understand what real scenario we cannot properly
> set them.
> 
> [Tuong]: Yes, these two variables have been initialized to zero at the link 
> create but zero or any other value is not 'safe' for them, the retransmit 
> failure criteria can be met without a real failure. Specifically, the 
> 'time_after()' can return 'true' unexpectedly due to a garbage value (like 
> zero...) of the 'stale_limit' unless it's intentionally set in the 1st 
> condition. However, the 1st condition with the 'prev_from' is not always 
> satisfied. In case the next 'from' is coincidentally identical to the 
> 'prev_from', we will miss it.
> 
>> -if (r->prev_from != from) {
>> -r->prev_from = from;
>> -r->stale_limit = jiffies + msecs_to_jiffies(r->tolerance);
>> -} else if (time_after(jiffies, r->stale_limit)) {
>> -pr_warn("Retransmission failure on link <%s>\n", l->name);
> 
> For example:
> 1) n-th retransmit: (prev_from = x, from = 100)
> ==> ok, we set the variables: prev_from = 100, stale_limit = jiffies + 1.5s
> 2) now, this pkt #100 was retransmitted successfully...
> 3) Later on, n+1-th retransmit: (prev_from = 100, from = 100)
> -> We don’t set the 'stale_limit' but do the “else if” and bump!
> 
> Now, we can try to reset or re-initialize the 'prev_from' somehow, e.g. when 
> the pkt #100 is ack-ed & released, but what value will be for it? Note, any 
> value is a valid sequence number, and if the next 'from' equals that value, 
> we will face with the same trouble again.
> Trying to set the 'stale_limit' to very far in the future is irrelevant too 
> because it turns out that we will disable the feature if the same 'from' is 
> really faced with the repeated retransmit failure!
> 
>>
>> Instead, a jiffies variable will be added to individual skbs (like the
>> way we restrict the skb retransmissions) in order to mark the first skb
>> retransmit time. Later on, at the next retransmissions, the timestamp
>> will be checked to see if the skb in the link transmq is "too stale",
>> that is, the link tolerance time has passed, so that a link reset will
>> be ordered. Note, just checking on the first skb in the queue is fine
>> enough since it must be the oldest one.
>> A counter is also added to keep track the actual skb retransmissions'
>> number for later checking when the failure happens.
>>
>> The downside of this approach is that the skb->cb[] buffer is about to
>> be exhausted, however it is always able to allocate another memory area
>> and keep a reference to it when needed.
>>
>> Fixes: 77cf8edbc0e7 ("tipc: simplify stale link failure criteria")
>> Reported-by: Hoang Le 
>> Signed-off-by: Tuong Lien 
>> ---
>>  net/tipc/link.c | 92 
>> -
>>  net/tipc/msg.h  |  8 +++--
>>  2 files changed, 57 insertions(+), 43 deletions(-)
>>
>> diff --git a/net/tipc/link.c b/net/tipc/link.c
>> index 66d3a07bc571..c2c5c53cad22 100644
>> --- a/net/tipc/link.c
>> +++ b/net/tipc/link.c
>> @@ -106,8 +106,6 @@ struct tipc_stats {
>>   * @transmitq: queue for sent, non-acked messages
>>   * @backlogq: queue for messages waiting to be sent
>>   * @snt_nxt: next sequence number to use for outbound messages
>> - * @prev_from: sequence number of most previous retransmission request
>> - 

Re: [tipc-discussion] memory leak in tipc_group_create_member

[Ying Xue]

On 8/2/19 3:44 PM, Hillf Danton wrote:
> 
> On Thu, 01 Aug 2019 19:38:06 -0700
>> Hello,
>>
>> syzbot found the following crash on:
>>
>> HEAD commit:a9815a4f Merge branch 'x86-urgent-for-linus' of git://git...
>> git tree:   upstream
>> console output: https://syzkaller.appspot.com/x/log.txt?x=12a6dbf060
>> kernel config:  https://syzkaller.appspot.com/x/.config?x=37c48fb52e3789e6
>> dashboard link: https://syzkaller.appspot.com/bug?extid=f95d90c454864b3b5bc9
>> compiler:   gcc (GCC) 9.0.0 20181231 (experimental)
>> syz repro:  https://syzkaller.appspot.com/x/repro.syz?x=13be3ecc60
>> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=11c992b460
>>
>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>> Reported-by: syzbot+f95d90c454864b3b5...@syzkaller.appspotmail.com
>>
>> executing program
>> BUG: memory leak
>> unreferenced object 0x888122bca200 (size 128):
>>comm "syz-executor232", pid 7065, jiffies 4294943817 (age 8.880s)
>>hex dump (first 32 bytes):
>>  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
>>  00 00 00 00 00 00 00 00 18 a2 bc 22 81 88 ff ff  ..."
>>backtrace:
>>  [<5bada299>] kmemleak_alloc_recursive  
>> include/linux/kmemleak.h:43 [inline]
>>  [<5bada299>] slab_post_alloc_hook mm/slab.h:522 [inline]
>>  [<5bada299>] slab_alloc mm/slab.c:3319 [inline]
>>  [<5bada299>] kmem_cache_alloc_trace+0x145/0x2c0 mm/slab.c:3548
>>  [<e7bcdc9f>] kmalloc include/linux/slab.h:552 [inline]
>>  [<e7bcdc9f>] kzalloc include/linux/slab.h:748 [inline]
>>  [<e7bcdc9f>] tipc_group_create_member+0x3c/0x190  
>> net/tipc/group.c:306
>>  [<05f56f40>] tipc_group_add_member+0x34/0x40  
>> net/tipc/group.c:327
>>  [<44406683>] tipc_nametbl_build_group+0x9b/0xf0  
>> net/tipc/name_table.c:600
>>  [<9f71e803>] tipc_sk_join net/tipc/socket.c:2901 [inline]
>>  [<9f71e803>] tipc_setsockopt+0x170/0x490 net/tipc/socket.c:3006
>>  [<7f61cbc2>] __sys_setsockopt+0x10f/0x220 net/socket.c:2084
>>  [<cc630372>] __do_sys_setsockopt net/socket.c:2100 [inline]
>>  [<cc630372>] __se_sys_setsockopt net/socket.c:2097 [inline]
>>  [<cc630372>] __x64_sys_setsockopt+0x26/0x30 net/socket.c:2097
>>  [<ec30be33>] do_syscall_64+0x76/0x1a0  
>> arch/x86/entry/common.c:296
>>  [<271be3e6>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>

Acked-by: Ying Xue 

>  
> --- a/net/tipc/group.c
> +++ b/net/tipc/group.c
> @@ -273,7 +273,7 @@ static struct tipc_member *tipc_group_fi
>   return NULL;
>  }
>  
> -static void tipc_group_add_to_tree(struct tipc_group *grp,
> +static struct tipc_member *tipc_group_add_to_tree(struct tipc_group *grp,
>  struct tipc_member *m)
>  {
>   u64 nkey, key = (u64)m->node << 32 | m->port;
> @@ -282,7 +282,6 @@ static void tipc_group_add_to_tree(struc
>  
>   n = >members.rb_node;
>   while (*n) {
> - tmp = container_of(*n, struct tipc_member, tree_node);
>   parent = *n;
>   tmp = container_of(parent, struct tipc_member, tree_node);
>   nkey = (u64)tmp->node << 32 | tmp->port;
> @@ -291,17 +290,18 @@ static void tipc_group_add_to_tree(struc
>   else if (key > nkey)
>   n = &(*n)->rb_right;
>   else
> - return;
> + return tmp;
>   }
>   rb_link_node(>tree_node, parent, n);
>   rb_insert_color(>tree_node, >members);
> + return m;
>  }
>  
>  static struct tipc_member *tipc_group_create_member(struct tipc_group *grp,
>   u32 node, u32 port,
>   u32 instance, int state)
>  {
> - struct tipc_member *m;
> + struct tipc_member *m, *n;
>  
>   m = kzalloc(sizeof(*m), GFP_ATOMIC);
>   if (!m)
> @@ -315,10 +315,14 @@ static struct tipc_member *tipc_group_cr
>   m->instance = instance;
>   m->bc_acked = grp->bc_snd_nxt - 1;
>   grp->member_cnt++;
> - tipc_group_add_to_tree(grp, m);
> - tipc_nlist_add(>dests, m->node);
> - m->state = state;
> - return m;
> + n = tipc_group_add_to_tree(grp, m);
> + if (n == m) {
> + tipc_nlist_add(>dests, m->node);
> + m->state = state;
> + } else {
> + kfree(m);
> + }
> + return n;
>  }
>  
>  void tipc_group_add_member(struct tipc_group *grp, u32 node,
> --
> 
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH] tipc: compat: allow tipc commands without arguments

[Ying Xue]

On 7/30/19 6:15 AM, Taras Kondratiuk wrote:
> Commit 2753ca5d9009 ("tipc: fix uninit-value in tipc_nl_compat_doit")
> broke older tipc tools that use compat interface (e.g. tipc-config from
> tipcutils package):
> 
> % tipc-config -p
> operation not supported
> 
> The commit started to reject TIPC netlink compat messages that do not
> have attributes. It is too restrictive because some of such messages are
> valid (they don't need any arguments):
> 
> % grep 'tx none' include/uapi/linux/tipc_config.h
> #define  TIPC_CMD_NOOP  0x/* tx none, rx none */
> #define  TIPC_CMD_GET_MEDIA_NAMES   0x0002/* tx none, rx media_name(s) */
> #define  TIPC_CMD_GET_BEARER_NAMES  0x0003/* tx none, rx bearer_name(s) */
> #define  TIPC_CMD_SHOW_PORTS0x0006/* tx none, rx ultra_string */
> #define  TIPC_CMD_GET_REMOTE_MNG0x4003/* tx none, rx unsigned */
> #define  TIPC_CMD_GET_MAX_PORTS 0x4004/* tx none, rx unsigned */
> #define  TIPC_CMD_GET_NETID 0x400B/* tx none, rx unsigned */
> #define  TIPC_CMD_NOT_NET_ADMIN 0xC001/* tx none, rx none */
> 
> This patch relaxes the original fix and rejects messages without
> arguments only if such arguments are expected by a command (reg_type is
> non zero).
> 
> Fixes: 2753ca5d9009 ("tipc: fix uninit-value in tipc_nl_compat_doit")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Taras Kondratiuk 

Acked-by: Ying Xue 

> ---
> The patch is based on v5.3-rc2.
> 
>  net/tipc/netlink_compat.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
> index d86030ef1232..e135d4e11231 100644
> --- a/net/tipc/netlink_compat.c
> +++ b/net/tipc/netlink_compat.c
> @@ -55,6 +55,7 @@ struct tipc_nl_compat_msg {
>   int rep_type;
>   int rep_size;
>   int req_type;
> + int req_size;
>   struct net *net;
>   struct sk_buff *rep;
>   struct tlv_desc *req;
> @@ -257,7 +258,8 @@ static int tipc_nl_compat_dumpit(struct 
> tipc_nl_compat_cmd_dump *cmd,
>   int err;
>   struct sk_buff *arg;
>  
> - if (msg->req_type && !TLV_CHECK_TYPE(msg->req, msg->req_type))
> + if (msg->req_type && (!msg->req_size ||
> +   !TLV_CHECK_TYPE(msg->req, msg->req_type)))
>   return -EINVAL;
>  
>   msg->rep = tipc_tlv_alloc(msg->rep_size);
> @@ -354,7 +356,8 @@ static int tipc_nl_compat_doit(struct 
> tipc_nl_compat_cmd_doit *cmd,
>  {
>   int err;
>  
> - if (msg->req_type && !TLV_CHECK_TYPE(msg->req, msg->req_type))
> + if (msg->req_type && (!msg->req_size ||
> +   !TLV_CHECK_TYPE(msg->req, msg->req_type)))
>   return -EINVAL;
>  
>   err = __tipc_nl_compat_doit(cmd, msg);
> @@ -1278,8 +1281,8 @@ static int tipc_nl_compat_recv(struct sk_buff *skb, 
> struct genl_info *info)
>   goto send;
>   }
>  
> - len = nlmsg_attrlen(req_nlh, GENL_HDRLEN + TIPC_GENL_HDRLEN);
> - if (!len || !TLV_OK(msg.req, len)) {
> + msg.req_size = nlmsg_attrlen(req_nlh, GENL_HDRLEN + TIPC_GENL_HDRLEN);
> + if (msg.req_size && !TLV_OK(msg.req, msg.req_size)) {
>   msg.rep = tipc_get_err_tlv(TIPC_CFG_NOT_SUPPORTED);
>   err = -EOPNOTSUPP;
>   goto send;
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net-next v3] tipc: add loopback device tracking

[Ying Xue]

On 7/19/19 11:10 AM, john.rutherf...@dektech.com.au wrote:
> From: John Rutherford 
> 
> Since node internal messages are passed directly to the socket, it is not
> possible to observe those messages via tcpdump or wireshark.
> 
> We now remedy this by making it possible to clone such messages and send
> the clones to the loopback interface.  The clones are dropped at reception
> and have no functional role except making the traffic visible.
> 
> The feature is enabled if network taps are active for the loopback device.
> pcap filtering restrictions require the messages to be presented to the
> receiving side of the loopback device.
> 
> v3 - Function dev_nit_active used to check for network taps.
>- Procedure netif_rx_ni used to send cloned messages to loopback device.
> 
> Signed-off-by: John Rutherford 

Acked-by: Ying Xue 

> ---
>  net/tipc/bcast.c  |  4 +++-
>  net/tipc/bearer.c | 65 
> +++
>  net/tipc/bearer.h | 10 +
>  net/tipc/core.c   |  5 +
>  net/tipc/core.h   |  3 +++
>  net/tipc/node.c   |  1 +
>  net/tipc/topsrv.c |  2 ++
>  7 files changed, 89 insertions(+), 1 deletion(-)
> 
> diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
> index 6c997d4..235331d 100644
> --- a/net/tipc/bcast.c
> +++ b/net/tipc/bcast.c
> @@ -406,8 +406,10 @@ int tipc_mcast_xmit(struct net *net, struct sk_buff_head 
> *pkts,
>   rc = tipc_bcast_xmit(net, pkts, cong_link_cnt);
>   }
>  
> - if (dests->local)
> + if (dests->local) {
> + tipc_loopback_trace(net, );
>   tipc_sk_mcast_rcv(net, , );
> + }
>  exit:
>   /* This queue should normally be empty by now */
>   __skb_queue_purge(pkts);
> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> index 2bed658..5273ec5 100644
> --- a/net/tipc/bearer.c
> +++ b/net/tipc/bearer.c
> @@ -389,6 +389,12 @@ int tipc_enable_l2_media(struct net *net, struct 
> tipc_bearer *b,
>   dev_put(dev);
>   return -EINVAL;
>   }
> + if (dev == net->loopback_dev) {
> + dev_put(dev);
> + pr_info("Bearer <%s>: loopback device not permitted\n",
> + b->name);
> + return -EINVAL;
> + }
>  
>   /* Autoconfigure own node identity if needed */
>   if (!tipc_own_id(net) && hwaddr_len <= NODE_ID_LEN) {
> @@ -674,6 +680,65 @@ void tipc_bearer_stop(struct net *net)
>   }
>  }
>  
> +void tipc_clone_to_loopback(struct net *net, struct sk_buff_head *pkts)
> +{
> + struct net_device *dev = net->loopback_dev;
> + struct sk_buff *skb, *_skb;
> + int exp;
> +
> + skb_queue_walk(pkts, _skb) {
> + skb = pskb_copy(_skb, GFP_ATOMIC);
> + if (!skb)
> + continue;
> +
> + exp = SKB_DATA_ALIGN(dev->hard_header_len - skb_headroom(skb));
> + if (exp > 0 && pskb_expand_head(skb, exp, 0, GFP_ATOMIC)) {
> + kfree_skb(skb);
> + continue;
> + }
> +
> + skb_reset_network_header(skb);
> + dev_hard_header(skb, dev, ETH_P_TIPC, dev->dev_addr,
> + dev->dev_addr, skb->len);
> + skb->dev = dev;
> + skb->pkt_type = PACKET_HOST;
> + skb->ip_summed = CHECKSUM_UNNECESSARY;
> + skb->protocol = eth_type_trans(skb, dev);
> + netif_rx_ni(skb);
> + }
> +}
> +
> +static int tipc_loopback_rcv_pkt(struct sk_buff *skb, struct net_device *dev,
> +  struct packet_type *pt, struct net_device *od)
> +{
> + consume_skb(skb);
> + return NET_RX_SUCCESS;
> +}
> +
> +int tipc_attach_loopback(struct net *net)
> +{
> + struct net_device *dev = net->loopback_dev;
> + struct tipc_net *tn = tipc_net(net);
> +
> + if (!dev)
> + return -ENODEV;
> +
> + dev_hold(dev);
> + tn->loopback_pt.dev = dev;
> + tn->loopback_pt.type = htons(ETH_P_TIPC);
> + tn->loopback_pt.func = tipc_loopback_rcv_pkt;
> + dev_add_pack(>loopback_pt);
> + return 0;
> +}
> +
> +void tipc_detach_loopback(struct net *net)
> +{
> + struct tipc_net *tn = tipc_net(net);
> +
> + dev_remove_pack(>loopback_pt);
> + dev_put(net->loopback_dev);
> +}
> +
>  /* Caller should hold rtnl_lock to protect the bearer */
>  static int __tipc_nl_add_bearer(struct tipc_nl_msg *msg,
>   struct tipc_bearer *bearer, int nlflags)
> 

Re: [tipc-discussion] [net] tipc: fix false detection of retransmit failures

[Ying Xue]

On 7/19/19 11:56 AM, Tuong Lien wrote:
> This commit eliminates the use of the link 'stale_limit' & 'prev_from'
> (besides the already removed - 'stale_cnt') variables in the detection
> of repeated retransmit failures as there is no proper way to initialize
> them to avoid a false detection, i.e. it is not really a retransmission
> failure but due to a garbage values in the variables.

Sorry, I couldn't understand the reason why we have no proper way to
initialize 'stale_limit' & 'prev_from' variables of tipc_link struct.

As far as I understand, the two variables have been set to zero when
tipc_link object is allocated with kzalloc() in tipc_link_create().

Can you please help me understand what real scenario we cannot properly
set them.

> 
> Instead, a jiffies variable will be added to individual skbs (like the
> way we restrict the skb retransmissions) in order to mark the first skb
> retransmit time. Later on, at the next retransmissions, the timestamp
> will be checked to see if the skb in the link transmq is "too stale",
> that is, the link tolerance time has passed, so that a link reset will
> be ordered. Note, just checking on the first skb in the queue is fine
> enough since it must be the oldest one.
> A counter is also added to keep track the actual skb retransmissions'
> number for later checking when the failure happens.
> 
> The downside of this approach is that the skb->cb[] buffer is about to
> be exhausted, however it is always able to allocate another memory area
> and keep a reference to it when needed.
> 
> Fixes: 77cf8edbc0e7 ("tipc: simplify stale link failure criteria")
> Reported-by: Hoang Le 
> Signed-off-by: Tuong Lien 
> ---
>  net/tipc/link.c | 92 
> -
>  net/tipc/msg.h  |  8 +++--
>  2 files changed, 57 insertions(+), 43 deletions(-)
> 
> diff --git a/net/tipc/link.c b/net/tipc/link.c
> index 66d3a07bc571..c2c5c53cad22 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -106,8 +106,6 @@ struct tipc_stats {
>   * @transmitq: queue for sent, non-acked messages
>   * @backlogq: queue for messages waiting to be sent
>   * @snt_nxt: next sequence number to use for outbound messages
> - * @prev_from: sequence number of most previous retransmission request
> - * @stale_limit: time when repeated identical retransmits must force link 
> reset
>   * @ackers: # of peers that needs to ack each packet before it can be 
> released
>   * @acked: # last packet acked by a certain peer. Used for broadcast.
>   * @rcv_nxt: next sequence number to expect for inbound messages
> @@ -164,9 +162,7 @@ struct tipc_link {
>   u16 limit;
>   } backlog[5];
>   u16 snd_nxt;
> - u16 prev_from;
>   u16 window;
> - unsigned long stale_limit;
>  
>   /* Reception */
>   u16 rcv_nxt;
> @@ -1044,47 +1040,53 @@ static void tipc_link_advance_backlog(struct 
> tipc_link *l,
>   * link_retransmit_failure() - Detect repeated retransmit failures
>   * @l: tipc link sender
>   * @r: tipc link receiver (= l in case of unicast)
> - * @from: seqno of the 1st packet in retransmit request
>   * @rc: returned code
>   *
>   * Return: true if the repeated retransmit failures happens, otherwise
>   * false
>   */
>  static bool link_retransmit_failure(struct tipc_link *l, struct tipc_link *r,
> - u16 from, int *rc)
> + int *rc)
>  {
>   struct sk_buff *skb = skb_peek(>transmq);
>   struct tipc_msg *hdr;
>  
>   if (!skb)
>   return false;
> - hdr = buf_msg(skb);
>  
> - /* Detect repeated retransmit failures on same packet */
> - if (r->prev_from != from) {
> - r->prev_from = from;
> - r->stale_limit = jiffies + msecs_to_jiffies(r->tolerance);
> - } else if (time_after(jiffies, r->stale_limit)) {
> - pr_warn("Retransmission failure on link <%s>\n", l->name);
> - link_print(l, "State of link ");
> - pr_info("Failed msg: usr %u, typ %u, len %u, err %u\n",
> - msg_user(hdr), msg_type(hdr), msg_size(hdr),
> - msg_errcode(hdr));
> - pr_info("sqno %u, prev: %x, src: %x\n",
> - msg_seqno(hdr), msg_prevnode(hdr), msg_orignode(hdr));
> -
> - trace_tipc_list_dump(>transmq, true, "retrans failure!");
> - trace_tipc_link_dump(l, TIPC_DUMP_NONE, "retrans failure!");
> - trace_tipc_link_dump(r, TIPC_DUMP_NONE, "retrans failure!");
> + if (!TIPC_SKB_CB(skb)->retr_cnt)
> + return false;
>  
> - if (link_is_bc_sndlink(l))
> - *rc = TIPC_LINK_DOWN_EVT;
> + if (!time_after(jiffies, TIPC_SKB_CB(skb)->retr_stamp +
> + msecs_to_jiffies(r->tolerance)))
> + return false;
> +
> + hdr = buf_msg(skb);
> + if (link_is_bc_sndlink(l) && !less(r->acked, msg_seqno(hdr)))
> + return 

Re: [tipc-discussion] [PATCH] net: tipc: Fix a possible null-pointer dereference in tipc_publ_purge()

[Ying Xue]

On 7/25/19 5:20 PM, Jia-Ju Bai wrote:
> In tipc_publ_purge(), there is an if statement on 215 to 
> check whether p is NULL: 
> if (p)
> 
> When p is NULL, it is used on line 226:
> kfree_rcu(p, rcu);
> 
> Thus, a possible null-pointer dereference may occur.
> 
> To fix this bug, p is checked before being used.
> 
> This bug is found by a static analysis tool STCheck written by us.
> 
> Signed-off-by: Jia-Ju Bai 
> ---
>  net/tipc/name_distr.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
> index 44abc8e9c990..241ed2274473 100644
> --- a/net/tipc/name_distr.c
> +++ b/net/tipc/name_distr.c
> @@ -223,7 +223,8 @@ static void tipc_publ_purge(struct net *net, struct 
> publication *publ, u32 addr)
>  publ->key);
>   }
>  
> - kfree_rcu(p, rcu);
> + if (p)

No, I don't think so because kfree_rcu() will internally check if "p"
pointer is NULL or not.

> + kfree_rcu(p, rcu);
>  }
>  
>  /**
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] TIPC ; config trouble ; help request

[Ying Xue]

On 7/13/19 11:58 AM, Mahesh Kumar via tipc-discussion wrote:
> Tipc Team,
> 
> Greetings!.
> I have been using TIPC for about a year without any issueHowever the TIPC 
> tool is bailing out when I tried to set address, bearer
> 
> 
> / # tipc node set addr 1.1.100
> 
> error: Operation not permitted
> 
> / # tipc bearer enable media eth dev ieobc
> 
> error: Invalid argument
> 
> / #
> 
> I am using the new kernel now;
>  uname -aLinux 2c3f0b001900_1_RP_0 4.4.180 #1 SMP Tue Jun 25 15:36:10 PDT 
> 2019 x86_64 x86_64 x86_64 GNU/Linux
>  dmesg output ; grep -i TIPC d.txt[   29.436599] tipc: Activated (version 
> 2.0.0)[   29.436768] tipc: Started in single node mode

Suspected some TIPC patches integrated through 4.4.180 release
introduced regression. The most simplest method to identify the issue is
to revert some TIPC patches to identify which ones caused the regression.

> 
> [2c3f0b001900_1_RP_0:/]$ tipc-config -ntType       Lower      Upper      Port 
> Identity              Publication Scope0          16781313   16781313   
> <1.1.1:0>                  16781313    zone1          1          1          
> <1.1.1:483390874>          483390875   node1          88         88         
> <1.1.1:2870943326>         2870943327  zone15003      5          5          
> <1.1.1:3556781096>         3556781097  zone[2c3f0b001900_1_RP_0:/]$ 
> 
> please let me know if any issue.
> thanks & regardsMahesh kumar.L
> 
> 
> ___
> tipc-discussion mailing list
> tipc-discussion@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net-next 1/1] tipc: embed jiffies in macro TIPC_BC_RETR_LIM

[Ying Xue]

On 6/28/19 11:06 PM, Jon Maloy wrote:
> The macro TIPC_BC_RETR_LIM is always used in combination with 'jiffies',
> so we can just as well perform the addition in the macro itself. This
> way, we get a few shorter code lines and one less line break.
> 
> Signed-off-by: Jon Maloy 

Acked-by: Ying Xue 

> ---
>  net/tipc/link.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/net/tipc/link.c b/net/tipc/link.c
> index f8bf63b..66d3a07 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -207,7 +207,7 @@ enum {
>   BC_NACK_SND_SUPPRESS,
>  };
>  
> -#define TIPC_BC_RETR_LIM msecs_to_jiffies(10)   /* [ms] */
> +#define TIPC_BC_RETR_LIM  (jiffies + msecs_to_jiffies(10))
>  #define TIPC_UC_RETR_TIME (jiffies + msecs_to_jiffies(1))
>  
>  /*
> @@ -976,8 +976,7 @@ int tipc_link_xmit(struct tipc_link *l, struct 
> sk_buff_head *list,
>   __skb_queue_tail(transmq, skb);
>   /* next retransmit attempt */
>   if (link_is_bc_sndlink(l))
> - TIPC_SKB_CB(skb)->nxt_retr =
> - jiffies + TIPC_BC_RETR_LIM;
> + TIPC_SKB_CB(skb)->nxt_retr = TIPC_BC_RETR_LIM;
>   __skb_queue_tail(xmitq, _skb);
>   TIPC_SKB_CB(skb)->ackers = l->ackers;
>   l->rcv_unacked = 0;
> @@ -1027,7 +1026,7 @@ static void tipc_link_advance_backlog(struct tipc_link 
> *l,
>   __skb_queue_tail(>transmq, skb);
>   /* next retransmit attempt */
>   if (link_is_bc_sndlink(l))
> - TIPC_SKB_CB(skb)->nxt_retr = jiffies + TIPC_BC_RETR_LIM;
> + TIPC_SKB_CB(skb)->nxt_retr = TIPC_BC_RETR_LIM;
>  
>   __skb_queue_tail(xmitq, _skb);
>   TIPC_SKB_CB(skb)->ackers = l->ackers;
> @@ -1123,7 +1122,7 @@ static int tipc_link_bc_retrans(struct tipc_link *l, 
> struct tipc_link *r,
>   if (link_is_bc_sndlink(l)) {
>   if (time_before(jiffies, TIPC_SKB_CB(skb)->nxt_retr))
>   continue;
> - TIPC_SKB_CB(skb)->nxt_retr = jiffies + TIPC_BC_RETR_LIM;
> + TIPC_SKB_CB(skb)->nxt_retr = TIPC_BC_RETR_LIM;
>   }
>   _skb = __pskb_copy(skb, LL_MAX_HEADER + MIN_H_SIZE, GFP_ATOMIC);
>   if (!_skb)
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net-next v2] tipc: add loopback device tracking

[Ying Xue]

On 6/21/19 9:41 PM, Jon Maloy wrote:
> Hi Ying,
> We discussed this, and even had a first version where we did this, performing 
> the whole attach/detach tasks in the command execution itself. This resulted 
> in way to much new code to my taste. 
> My greatest concern now is that some users will enable this interface because 
> they think it is necessary (we have already seen cases of that) and then 
> start complaining about performance.
> So, as a compromise, I suggested we could add a "confirmation" printout in 
> the tipc tool when the loopback interface is enabled, to make the user 
> understand that this is for tracing only.
> Do you think this would be acceptable?

Make sense to me.

Thanks,
Ying

> 
> ///jon
> 
> 
>> -Original Message-
>> From: Ying Xue 
>> Sent: 21-Jun-19 08:16
>> To: John Rutherford ; tipc-
>> discuss...@lists.sourceforge.net
>> Subject: Re: [tipc-discussion] [net-next v2] tipc: add loopback device 
>> tracking
>>
>> Good work!
>>
>> Just one suggestion: it's better to add one separate kernel config to control
>> whether the new feature is enabled or not, and its default value should be 
>> set
>> to "Disabled" because the feature is related to debug.
>>
>> On 6/19/19 8:11 AM, John Rutherford wrote:
>>> Since node internal messages are passed directly to socket it is not
>>> possible to observe this message exchange via tcpdump or wireshark.
>>>
>>> We now remedy this by making it possible to clone such messages and
>>> send the clones to the loopback interface.  The clones are dropped at
>>> reception and have no functional role except making the traffic visible.
>>>
>>> The feature is turned on/off by enabling/disabling the loopback "bearer"
>>> "eth:lo".
>>>
>>> Signed-off-by: John Rutherford 
>>> ---
>>>  net/tipc/bcast.c  |  4 +++-
>>>  net/tipc/bearer.c | 67
>>> +++
>>>  net/tipc/bearer.h |  3 +++
>>>  net/tipc/core.c   |  5 -
>>>  net/tipc/core.h   | 12 ++
>>>  net/tipc/node.c   |  1 +
>>>  net/tipc/topsrv.c |  2 ++
>>>  7 files changed, 92 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c index
>>> 6c997d4..235331d 100644
>>> --- a/net/tipc/bcast.c
>>> +++ b/net/tipc/bcast.c
>>> @@ -406,8 +406,10 @@ int tipc_mcast_xmit(struct net *net, struct
>> sk_buff_head *pkts,
>>> rc = tipc_bcast_xmit(net, pkts, cong_link_cnt);
>>> }
>>>
>>> -   if (dests->local)
>>> +   if (dests->local) {
>>> +   tipc_loopback_trace(net, );
>>> tipc_sk_mcast_rcv(net, , );
>>> +   }
>>>  exit:
>>> /* This queue should normally be empty by now */
>>> __skb_queue_purge(pkts);
>>> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c index
>>> 2bed658..27b4fd7 100644
>>> --- a/net/tipc/bearer.c
>>> +++ b/net/tipc/bearer.c
>>> @@ -836,6 +836,12 @@ int __tipc_nl_bearer_disable(struct sk_buff *skb,
>>> struct genl_info *info)
>>>
>>> name = nla_data(attrs[TIPC_NLA_BEARER_NAME]);
>>>
>>> +   if (!strcmp(name, "eth:lo")) {
>>> +   tipc_net(net)->loopback_trace = false;
>>> +   pr_info("Disabled packet tracing on loopback interface\n");
>>> +   return 0;
>>> +   }
>>> +
>>> bearer = tipc_bearer_find(net, name);
>>> if (!bearer)
>>> return -EINVAL;
>>> @@ -881,6 +887,12 @@ int __tipc_nl_bearer_enable(struct sk_buff *skb,
>>> struct genl_info *info)
>>>
>>> bearer = nla_data(attrs[TIPC_NLA_BEARER_NAME]);
>>>
>>> +   if (!strcmp(bearer, "eth:lo")) {
>>> +   tipc_net(net)->loopback_trace = true;
>>> +   pr_info("Enabled packet tracing on loopback interface\n");
>>> +   return 0;
>>> +   }
>>> +
>>> if (attrs[TIPC_NLA_BEARER_DOMAIN])
>>> domain = nla_get_u32(attrs[TIPC_NLA_BEARER_DOMAIN]);
>>>
>>> @@ -1021,6 +1033,61 @@ int tipc_nl_bearer_set(struct sk_buff *skb,
>> struct genl_info *info)
>>> return err;
>>>  }
>>>
>>> +void tipc_clone_to_loopback(struct net *net, struct sk_buff_head
>>> +*xmitq) {
>>> +   struct net_device *dev = net-&g

Re: [tipc-discussion] [net-next v2] tipc: add loopback device tracking

[Ying Xue]

Good work!

Just one suggestion: it's better to add one separate kernel config to
control whether the new feature is enabled or not, and its default value
should be set to "Disabled" because the feature is related to debug.

On 6/19/19 8:11 AM, John Rutherford wrote:
> Since node internal messages are passed directly to socket it is not
> possible to observe this message exchange via tcpdump or wireshark.
> 
> We now remedy this by making it possible to clone such messages and send
> the clones to the loopback interface.  The clones are dropped at reception
> and have no functional role except making the traffic visible.
> 
> The feature is turned on/off by enabling/disabling the loopback "bearer"
> "eth:lo".
>  
> Signed-off-by: John Rutherford 
> ---
>  net/tipc/bcast.c  |  4 +++-
>  net/tipc/bearer.c | 67 
> +++
>  net/tipc/bearer.h |  3 +++
>  net/tipc/core.c   |  5 -
>  net/tipc/core.h   | 12 ++
>  net/tipc/node.c   |  1 +
>  net/tipc/topsrv.c |  2 ++
>  7 files changed, 92 insertions(+), 2 deletions(-)
> 
> diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
> index 6c997d4..235331d 100644
> --- a/net/tipc/bcast.c
> +++ b/net/tipc/bcast.c
> @@ -406,8 +406,10 @@ int tipc_mcast_xmit(struct net *net, struct sk_buff_head 
> *pkts,
>   rc = tipc_bcast_xmit(net, pkts, cong_link_cnt);
>   }
>  
> - if (dests->local)
> + if (dests->local) {
> + tipc_loopback_trace(net, );
>   tipc_sk_mcast_rcv(net, , );
> + }
>  exit:
>   /* This queue should normally be empty by now */
>   __skb_queue_purge(pkts);
> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> index 2bed658..27b4fd7 100644
> --- a/net/tipc/bearer.c
> +++ b/net/tipc/bearer.c
> @@ -836,6 +836,12 @@ int __tipc_nl_bearer_disable(struct sk_buff *skb, struct 
> genl_info *info)
>  
>   name = nla_data(attrs[TIPC_NLA_BEARER_NAME]);
>  
> + if (!strcmp(name, "eth:lo")) {
> + tipc_net(net)->loopback_trace = false;
> + pr_info("Disabled packet tracing on loopback interface\n");
> + return 0;
> + }
> +
>   bearer = tipc_bearer_find(net, name);
>   if (!bearer)
>   return -EINVAL;
> @@ -881,6 +887,12 @@ int __tipc_nl_bearer_enable(struct sk_buff *skb, struct 
> genl_info *info)
>  
>   bearer = nla_data(attrs[TIPC_NLA_BEARER_NAME]);
>  
> + if (!strcmp(bearer, "eth:lo")) {
> + tipc_net(net)->loopback_trace = true;
> + pr_info("Enabled packet tracing on loopback interface\n");
> + return 0;
> + }
> +
>   if (attrs[TIPC_NLA_BEARER_DOMAIN])
>   domain = nla_get_u32(attrs[TIPC_NLA_BEARER_DOMAIN]);
>  
> @@ -1021,6 +1033,61 @@ int tipc_nl_bearer_set(struct sk_buff *skb, struct 
> genl_info *info)
>   return err;
>  }
>  
> +void tipc_clone_to_loopback(struct net *net, struct sk_buff_head *xmitq)
> +{
> + struct net_device *dev = net->loopback_dev;
> + struct sk_buff *skb, *_skb;
> + int exp;
> +
> + skb_queue_walk(xmitq, _skb) {
> + skb = pskb_copy(_skb, GFP_ATOMIC);
> + if (!skb)
> + continue;
> + exp = SKB_DATA_ALIGN(dev->hard_header_len - skb_headroom(skb));
> + if (exp > 0 && pskb_expand_head(skb, exp, 0, GFP_ATOMIC)) {
> + kfree_skb(skb);
> + continue;
> + }
> + skb_reset_network_header(skb);
> + skb->dev = dev;
> + skb->protocol = htons(ETH_P_TIPC);
> + dev_hard_header(skb, dev, ETH_P_TIPC, dev->dev_addr,
> + dev->dev_addr, skb->len);
> + dev_queue_xmit(skb);
> + }
> +}
> +
> +static int tipc_loopback_rcv_pkt(struct sk_buff *skb, struct net_device *dev,
> +  struct packet_type *pt, struct net_device *od)
> +{
> + consume_skb(skb);
> + return NET_RX_SUCCESS;
> +}
> +
> +int tipc_attach_loopback(struct net *net)
> +{
> + struct net_device *dev = net->loopback_dev;
> + struct tipc_net *tn = tipc_net(net);
> +
> + if (!dev)
> + return -ENODEV;
> + dev_hold(dev);
> + tn->loopback_pt.dev = dev;
> + tn->loopback_pt.type = htons(ETH_P_TIPC);
> + tn->loopback_pt.func = tipc_loopback_rcv_pkt;
> + tn->loopback_trace = false;
> + dev_add_pack(>loopback_pt);
> + return 0;
> +}
> +
> +void tipc_detach_loopback(struct net *net)
> +{
> + struct tipc_net *tn = tipc_net(net);
> +
> + dev_remove_pack(>loopback_pt);
> + dev_put(net->loopback_dev);
> +}
> +
>  static int __tipc_nl_add_media(struct tipc_nl_msg *msg,
>  struct tipc_media *media, int nlflags)
>  {
> diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
> index 7f4c569..ef7fad9 100644
> --- a/net/tipc/bearer.h
> +++ b/net/tipc/bearer.h
> @@ -232,6 +232,9 @@ void tipc_bearer_xmit(struct net *net, u32 bearer_id,

Re: [tipc-discussion] [PATCH net] tipc: purge deferredq list for each grp member in tipc_group_delete

[Ying Xue]

On 6/16/19 5:24 PM, Xin Long wrote:
> Syzbot reported a memleak caused by grp members' deferredq list not
> purged when the grp is be deleted.
> 
> The issue occurs when more(msg_grp_bc_seqno(hdr), m->bc_rcv_nxt) in
> tipc_group_filter_msg() and the skb will stay in deferredq.
> 
> So fix it by calling __skb_queue_purge for each member's deferredq
> in tipc_group_delete() when a tipc sk leaves the grp.
> 
> Fixes: b87a5ea31c93 ("tipc: guarantee group unicast doesn't bypass group 
> broadcast")
> Reported-by: syzbot+78fbe679c8ca8d264...@syzkaller.appspotmail.com
> Signed-off-by: Xin Long 

Acked-by: Ying Xue 

> ---
>  net/tipc/group.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/tipc/group.c b/net/tipc/group.c
> index 992be61..5f98d38 100644
> --- a/net/tipc/group.c
> +++ b/net/tipc/group.c
> @@ -218,6 +218,7 @@ void tipc_group_delete(struct net *net, struct tipc_group 
> *grp)
>  
>   rbtree_postorder_for_each_entry_safe(m, tmp, tree, tree_node) {
>   tipc_group_proto_xmit(grp, m, GRP_LEAVE_MSG, );
> + __skb_queue_purge(>deferredq);
>   list_del(>list);
>   kfree(m);
>   }
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH] tipc: add loopback device tracking

[Ying Xue]

Hi John,

Can you please submit the patch with "git send-email" command again?

The patch has been totally messed under Linux environment.

Thanks,
Ying

On 6/14/19 9:58 AM, John Rutherford wrote:
> Since node internal messages are passed directly to socket it is not
> possible to observe
> 
> this message exchange via tcpdump or wireshark.
> 
>  
> 
> We now remedy this by making it possible to clone such messages and send the
> clones to the
> 
> loopback interface.  The clones are dropped at reception and have no
> functional role except
> 
> making the traffic visible.
> 
>  
> 
> The feature is turned on/off by enabling/disabling the loopback "bearer"
> "eth:lo".
> 
> Signed-off-by: John Rutherford 
> 
> Signed-off-by: Jon Maloy 
> 
> ---
> 
> net/tipc/bcast.c   |  5 +++-
> 
> net/tipc/bearer.c  | 67
> ++
> 
> net/tipc/bearer.h  |  3 +++
> 
> net/tipc/core.c|  5 +++-
> 
> net/tipc/core.h|  9 
> 
> net/tipc/netlink.c |  2 +-
> 
> net/tipc/node.c|  2 ++
> 
> net/tipc/topsrv.c  |  3 +++
> 
> 8 files changed, 93 insertions(+), 3 deletions(-)
> 
>  
> 
> diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
> 
> index 6c997d4..db40129 100644
> 
> --- a/net/tipc/bcast.c
> 
> +++ b/net/tipc/bcast.c
> 
> @@ -406,8 +406,11 @@ int tipc_mcast_xmit(struct net *net, struct
> sk_buff_head *pkts,
> 
>rc = tipc_bcast_xmit(net,
> pkts, cong_link_cnt);
> 
>}
> 
> -  if (dests->local)
> 
> + if (dests->local) {
> 
> + if (unlikely(tipc_loopback_trace(net)))
> 
> + tipc_clone_to_loopback(net,
> );
> 
>tipc_sk_mcast_rcv(net, , );
> 
> + }
> 
> exit:
> 
>/* This queue should normally be empty by now */
> 
>__skb_queue_purge(pkts);
> 
> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> 
> index 2bed658..27b4fd7 100644
> 
> --- a/net/tipc/bearer.c
> 
> +++ b/net/tipc/bearer.c
> 
> @@ -836,6 +836,12 @@ int __tipc_nl_bearer_disable(struct sk_buff *skb,
> struct genl_info *info)
> 
> name = nla_data(attrs[TIPC_NLA_BEARER_NAME]);
> 
> + if (!strcmp(name, "eth:lo")) {
> 
> + tipc_net(net)->loopback_trace = false;
> 
> + pr_info("Disabled packet tracing on loopback
> interface\n");
> 
> + return 0;
> 
> + }
> 
> +
> 
>bearer = tipc_bearer_find(net, name);
> 
>if (!bearer)
> 
>return -EINVAL;
> 
> @@ -881,6 +887,12 @@ int __tipc_nl_bearer_enable(struct sk_buff *skb, struct
> genl_info *info)
> 
> bearer = nla_data(attrs[TIPC_NLA_BEARER_NAME]);
> 
> + if (!strcmp(bearer, "eth:lo")) {
> 
> + tipc_net(net)->loopback_trace = true;
> 
> + pr_info("Enabled packet tracing on loopback
> interface\n");
> 
> + return 0;
> 
> + }
> 
> +
> 
>if (attrs[TIPC_NLA_BEARER_DOMAIN])
> 
>domain =
> nla_get_u32(attrs[TIPC_NLA_BEARER_DOMAIN]);
> 
> @@ -1021,6 +1033,61 @@ int tipc_nl_bearer_set(struct sk_buff *skb, struct
> genl_info *info)
> 
>return err;
> 
> }
> 
> +void tipc_clone_to_loopback(struct net *net, struct sk_buff_head *xmitq)
> 
> +{
> 
> + struct net_device *dev = net->loopback_dev;
> 
> + struct sk_buff *skb, *_skb;
> 
> + int exp;
> 
> +
> 
> + skb_queue_walk(xmitq, _skb) {
> 
> + skb = pskb_copy(_skb, GFP_ATOMIC);
> 
> + if (!skb)
> 
> + continue;
> 
> + exp = SKB_DATA_ALIGN(dev->hard_header_len -
> skb_headroom(skb));
> 
> + if (exp > 0 && pskb_expand_head(skb, exp, 0,
> GFP_ATOMIC)) {
> 
> + kfree_skb(skb);
> 
> + continue;
> 
> + }
> 
> + skb_reset_network_header(skb);
> 
> + skb->dev = dev;
> 
> + skb->protocol = htons(ETH_P_TIPC);
> 
> + dev_hard_header(skb, dev, ETH_P_TIPC,
> dev->dev_addr,
> 
> + dev->dev_addr,
> skb->len);
> 
> + dev_queue_xmit(skb);
> 
> + }
> 
> +}
> 
> +
> 
> +static int tipc_loopback_rcv_pkt(struct sk_buff *skb, struct net_device
> *dev,
> 
> + struct
> packet_type *pt, struct net_device *od)
> 
> +{
> 
> + consume_skb(skb);
> 
> + 

Re: [tipc-discussion] memory leak in tipc_buf_acquire

[Ying Xue]

On 6/10/19 2:44 AM, Xin Long wrote:
> Looks we need to purge each member's deferredq list in tipc_group_delete():
> diff --git a/net/tipc/group.c b/net/tipc/group.c
> index 992be61..23823eb 100644
> --- a/net/tipc/group.c
> +++ b/net/tipc/group.c
> @@ -218,6 +218,7 @@ void tipc_group_delete(struct net *net, struct
> tipc_group *grp)
> 
>   rbtree_postorder_for_each_entry_safe(m, tmp, tree, tree_node) {
>   tipc_group_proto_xmit(grp, m, GRP_LEAVE_MSG, );
> + __skb_queue_purge(>deferredq);
>   list_del(>list);
>   kfree(m);
>   }

Good catch! I agree with you.


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH RFC 2/2] tipc: fix changeover issues due to large packet

[Ying Xue]

> 2) The same scenario above can happen more easily in case the MTU of
> the links is set differently or when changing. In that case, as long as
> a large message in the failure link's transmq queue was built and
> fragmented with its link's MTU > the other link's one, the issue will
> happen (there is no need of a link synching in advance).
> 
> 3) The link synching procedure also faces with the same issue but since
> the link synching is only started upon receipt of a SYNCH_MSG, dropping
> the message will not result in a state deadlock, but it is not expected
> as design.
> 
> The 1) & 3) issues are resolved by the previous commit 81e4dd94b214

This is the same as previous commit. The commit ID might be invalid
after it's merged into upstream.

> ("tipc: optimize link synching mechanism") by generating only a dummy
> SYNCH_MSG (i.e. without data) at the link synching, so the size of a
> FAILOVER_MSG if any then will never exceed the link's MTU.

>  /**
> + * tipc_msg_fragment - build a fragment skb list for TIPC message
> + *
> + * @skb: TIPC message skb
> + * @hdr: internal msg header to be put on the top of the fragments
> + * @pktmax: max size of a fragment incl. the header
> + * @frags: returned fragment skb list
> + *
> + * Returns 0 if the fragmentation is successful, otherwise: -EINVAL
> + * or -ENOMEM
> + */
> +int tipc_msg_fragment(struct sk_buff *skb, const struct tipc_msg *hdr,
> +   int pktmax, struct sk_buff_head *frags)
> +{
> + int pktno, nof_fragms, dsz, dmax, eat;
> + struct tipc_msg *_hdr;
> + struct sk_buff *_skb;
> + u8 *data;
> +
> + /* Non-linear buffer? */
> + if (skb_linearize(skb))
> + return -ENOMEM;
> +
> + data = (u8 *)skb->data;
> + dsz = msg_size(buf_msg(skb));
> + dmax = pktmax - INT_H_SIZE;
> +
> + if (dsz <= dmax || !dmax)
> + return -EINVAL;
> +
> + nof_fragms = dsz / dmax + 1;
> +
> + for (pktno = 1; pktno <= nof_fragms; pktno++) {
> + if (pktno < nof_fragms)
> + eat = dmax;
> + else
> + eat = dsz % dmax;
> +
> + _skb = tipc_buf_acquire(INT_H_SIZE + eat, GFP_ATOMIC);
> + if (!_skb)
> + goto error;
> +
> + skb_orphan(_skb);
> + __skb_queue_tail(frags, _skb);
> +
> + skb_copy_to_linear_data(_skb, hdr, INT_H_SIZE);
> + skb_copy_to_linear_data_offset(_skb, INT_H_SIZE, data, eat);
> + data += eat;
> +
> + _hdr = buf_msg(_skb);
> + msg_set_fragm_no(_hdr, pktno);
> + msg_set_nof_fragms(_hdr, nof_fragms);
> + msg_set_size(_hdr, INT_H_SIZE + eat);
> + }
> + return 0;
> +

In fact we have similar code in tipc_msg_build() where we also fragment
packet if necessary. In order to eliminate redundant code, I suggest we
should extract the common code into a separate function and then
tipc_msg_build() and tipc_msg_fragment() call it.



___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH v2] tipc: Avoid copying bytes beyond the supplied data

[Ying Xue]

On 5/23/19 4:46 AM, Chris Packham wrote:
> On most distros that is generated from include/uapi in the kernel source 
> and packaged as part of libc or a kernel-headers package. So once this 
> patch is accepted and makes it into the distros 
> /usr/include/linux/tipc_config.h will have this fix.

Thanks for the clarification. You are right, so it's unnecessary to make
any change.


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

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

[Ying Xue]

On 4/2/19 8:40 PM, Hoang Le wrote:
> skb somehow dequeued out of inputq before processing, it causes to
> NULL pointer and kernel crashed.
> 
> Add checking skb valid before using.
> 
> Fixes: c55c8edafa9 ("tipc: smooth change between replicast and broadcast")
> Reported-by: Tuong Lien Tong 
> Signed-off-by: Hoang Le 

Acked-by: Ying Xue 

> ---
>  net/tipc/bcast.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
> index 76e14dc08bb9..6c997d4a6218 100644
> --- a/net/tipc/bcast.c
> +++ b/net/tipc/bcast.c
> @@ -769,6 +769,9 @@ void tipc_mcast_filter_msg(struct net *net, struct 
> sk_buff_head *defq,
>   u32 node, port;
>  
>   skb = skb_peek(inputq);
> + if (!skb)
> + return;
> +
>   hdr = buf_msg(skb);
>  
>   if (likely(!msg_is_syn(hdr) && skb_queue_empty(defq)))
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH net] tipc: change to check tipc_own_id to return in tipc_net_stop

[Ying Xue]

On 3/24/19 12:48 AM, Xin Long wrote:
> When running a syz script, a panic occurred:
> 
> [  156.088228] BUG: KASAN: use-after-free in tipc_disc_timeout+0x9c9/0xb20 
> [tipc]
> [  156.094315] Call Trace:
> [  156.094844]  
> [  156.095306]  dump_stack+0x7c/0xc0
> [  156.097346]  print_address_description+0x65/0x22e
> [  156.100445]  kasan_report.cold.3+0x37/0x7a
> [  156.102402]  tipc_disc_timeout+0x9c9/0xb20 [tipc]
> [  156.106517]  call_timer_fn+0x19a/0x610
> [  156.112749]  run_timer_softirq+0xb51/0x1090
> 
> It was caused by the netns freed without deleting the discoverer timer,
> while later on the netns would be accessed in the timer handler.
> 
> The timer should have been deleted by tipc_net_stop() when cleaning up a
> netns. However, tipc has been able to enable a bearer and start d->timer
> without the local node_addr set since Commit 52dfae5c85a4 ("tipc: obtain
> node identity from interface by default"), which caused the timer not to
> be deleted in tipc_net_stop() then.
> 
> So fix it in tipc_net_stop() by changing to check local node_id instead
> of local node_addr, as Jon suggested.
> 
> While at it, remove the calling of tipc_nametbl_withdraw() there, since
> tipc_nametbl_stop() will take of the nametbl's freeing after.
> 
> Fixes: 52dfae5c85a4 ("tipc: obtain node identity from interface by default")
> Reported-by: syzbot+a25307ad099309f1c...@syzkaller.appspotmail.com
> Signed-off-by: Xin Long 

Acked-by: Ying Xue 

> ---
>  net/tipc/net.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/net/tipc/net.c b/net/tipc/net.c
> index f076edb..7ce1e86 100644
> --- a/net/tipc/net.c
> +++ b/net/tipc/net.c
> @@ -163,12 +163,9 @@ void tipc_sched_net_finalize(struct net *net, u32 addr)
>  
>  void tipc_net_stop(struct net *net)
>  {
> - u32 self = tipc_own_addr(net);
> -
> - if (!self)
> + if (!tipc_own_id(net))
>   return;
>  
> - tipc_nametbl_withdraw(net, TIPC_CFG_SRV, self, self, self);
>   rtnl_lock();
>   tipc_bearer_stop(net);
>   tipc_node_stop(net);
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

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

[Ying Xue]

Hi Tuong,

Great job! It's a very nice enhancement, and we should do the
improvement early.

On 3/20/19 11:28 AM, Tuong Lien wrote:
> During unicast link transmission, it's observed very often that because
> of one or a few lost/dis-ordered packets, the sending side will fastly
> reach the send window limit and must wait for the packets to be arrived
> at the receiving side or in the worst case, a retransmission must be
> done first. The sending side cannot release a lot of subsequent packets
> in its transmq even though all of them might have already been received
> by the receiving side.
> That is, one or two packets dis-ordered/lost and dozens of packets have
> to wait, this obviously reduces the overall throughput!
> 
> This commit introduces an algorithm to overcome this by using "Gap ACK
> blocks". Basically, a Gap ACK block will consist of  numbers
> that describes the link deferdq where packets have been got by the
> receiving side but with gaps, for example:
> 
>   link deferdq: [1 2 3 4  10 11  13 14 15   20]
> --> Gap ACK blocks:   <4, 5>,   <11, 1>,  <15, 4>, <20, 0>

This idea is the exactly same as SACK of SCTP:
https://tools.ietf.org/html/rfc4960#section-3.3.4


0   1   2   3
0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |   Type = 3|Chunk  Flags   |  Chunk Length |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |  Cumulative TSN Ack   |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |  Advertised Receiver Window Credit (a_rwnd)   |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   | Number of Gap Ack Blocks = N  |  Number of Duplicate TSNs = X |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |  Gap Ack Block #1 Start   |   Gap Ack Block #1 End|
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   /   /
   \  ...  \
   /   /
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |   Gap Ack Block #N Start  |  Gap Ack Block #N End |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |   Duplicate TSN 1 |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   /   /
   \  ...  \
   /   /
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |   Duplicate TSN X |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

   For example, assume that the receiver has the following DATA chunks
   newly arrived at the time when it decides to send a Selective ACK,

   --
   | TSN=17 |
   --
   || <- still missing
   --
   | TSN=15 |
   --
   | TSN=14 |
   --
   || <- still missing
   --
   | TSN=12 |
   --
   | TSN=11 |
   --
   | TSN=10 |
   --

   then the parameter part of the SACK MUST be constructed as follows
   (assuming the new a_rwnd is set to 4660 by the sender):

 ++
 |   Cumulative TSN Ack = 12  |
 ++
 |a_rwnd = 4660   |
 ++---+
 | num of block=2 | num of dup=0  |
 ++---+
 |block #1 strt=2 |block #1 end=3 |
 ++---+
 |block #2 strt=5 |block #2 end=5 |
 ++---+


It seems more easier to understand than our solution. Of course, I don't
intend to disagree to your proposal. In addition, we also can consider
to mark duplicated packet as SCTP is doing.

For duplicated packets, SCTP adapt the strategy:
  

Re: [tipc-discussion] [PATCH v1 2/2] tipc: improve function tipc_wait_for_rcvmsg()

[Ying Xue]

On 2/13/19 4:18 PM, Tung Nguyen wrote:
> This commit replaces schedule_timeout() with wait_woken()
> in function tipc_wait_for_rcvmsg(). wait_woken() uses
> memory barriers in its implementation to avoid potential
> race condition when putting a process into sleeping state
> and then waking it up.
> 
> Signed-off-by: Tung Nguyen 

Acked-by: Ying Xue 

> ---
>  net/tipc/socket.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index 81b87916a0eb..684f2125fc6b 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -1677,7 +1677,7 @@ static void tipc_sk_send_ack(struct tipc_sock *tsk)
>  static int tipc_wait_for_rcvmsg(struct socket *sock, long *timeop)
>  {
>   struct sock *sk = sock->sk;
> - DEFINE_WAIT(wait);
> + DEFINE_WAIT_FUNC(wait, woken_wake_function);
>   long timeo = *timeop;
>   int err = sock_error(sk);
>  
> @@ -1685,15 +1685,17 @@ static int tipc_wait_for_rcvmsg(struct socket *sock, 
> long *timeop)
>   return err;
>  
>   for (;;) {
> - prepare_to_wait(sk_sleep(sk), , TASK_INTERRUPTIBLE);
>   if (timeo && skb_queue_empty(>sk_receive_queue)) {
>   if (sk->sk_shutdown & RCV_SHUTDOWN) {
>   err = -ENOTCONN;
>   break;
>   }
> + add_wait_queue(sk_sleep(sk), );
>   release_sock(sk);
> - timeo = schedule_timeout(timeo);
> + timeo = wait_woken(, TASK_INTERRUPTIBLE, timeo);
> + sched_annotate_sleep();
>   lock_sock(sk);
> + remove_wait_queue(sk_sleep(sk), );
>   }
>   err = 0;
>   if (!skb_queue_empty(>sk_receive_queue))
> @@ -1709,7 +1711,6 @@ static int tipc_wait_for_rcvmsg(struct socket *sock, 
> long *timeop)
>   if (err)
>   break;
>   }
> - finish_wait(sk_sleep(sk), );
>   *timeop = timeo;
>   return err;
>  }
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH v1 1/2] tipc: improve function tipc_wait_for_cond()

[Ying Xue]

On 2/13/19 4:18 PM, Tung Nguyen wrote:
> Commit 844cf763fba654436d3a4279b6a672c196cf1901
> (tipc: make macro tipc_wait_for_cond() smp safe)

Please use the following words to describe commit info:

Commit 844cf763fba6 ("tipc: make macro tipc_wait_for_cond() smp safe")

 replaced
> finish_wait() with remove_wait_queue() but still used
> prepare_to_wait(). This causes unnecessary conditional
> checking  before adding to wait queue in prepare_to_wait().
> 
> This commit replaces prepare_to_wait() with add_wait_queue()
> as the pair function with remove_wait_queue().
> 
> Signed-off-by: Tung Nguyen 

Acked-by: Ying Xue 

> ---
>  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 1217c90a363b..81b87916a0eb 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -388,7 +388,7 @@ static int tipc_sk_sock_err(struct socket *sock, long 
> *timeout)
>   rc_ = tipc_sk_sock_err((sock_), timeo_);   \
>   if (rc_)   \
>   break; \
> - prepare_to_wait(sk_sleep(sk_), _, TASK_INTERRUPTIBLE);\
> + add_wait_queue(sk_sleep(sk_), _); \
>   release_sock(sk_); \
>   *(timeo_) = wait_woken(_, TASK_INTERRUPTIBLE, *(timeo_)); \
>   sched_annotate_sleep();\
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH] tipc: fix link session and re-establish issues

[Ying Xue]

On 1/24/19 10:50 AM, Tuong Lien wrote:
> When a link endpoint is re-created (e.g. after a node reboot or
> interface reset), the link session number is varied by random, the peer
> endpoint will be synced with this new session number before the link is
> re-established.
> 
> However, there is a shortcoming in this mechanism that can lead to the
> link never re-established or faced with a failure then. It happens when
> the peer endpoint is ready in ESTABLISHING state, the 'peer_session' as
> well as the 'in_session' flag have been set, but suddenly this link
> endpoint leaves. When it comes back with a random session number, there
> are two situations possible:
> 
> 1/ If the random session number is larger than (or equal to) the
> previous one, the peer endpoint will be updated with this new session
> upon receipt of a RESET_MSG from this endpoint, and the link can be re-
> established as normal. Otherwise, all the RESET_MSGs from this endpoint
> will be rejected by the peer. In turn, when this link endpoint receives
> one ACTIVATE_MSG from the peer, it will move to ESTABLISHED and start
> to send STATE_MSGs, but again these messages will be dropped by the
> peer due to wrong session.
> The peer link endpoint can still become ESTABLISHED after receiving a
> traffic message from this endpoint (e.g. a BCAST_PROTOCOL or
> NAME_DISTRIBUTOR), but since all the STATE_MSGs are invalid, the link
> will be forced down sooner or later!
> 
> Even in case the random session number is larger than the previous one,
> it can be that the ACTIVATE_MSG from the peer arrives first, and this
> link endpoint moves quickly to ESTABLISHED without sending out any
> RESET_MSG yet. Consequently, the peer link will not be updated with the
> new session number, and the same link failure scenario as above will
> happen.
> 
> 2/ Another situation can be that, the peer link endpoint was reset due
> to any reasons in the meantime, its link state was set to RESET from
> ESTABLISHING but still in session, i.e. the 'in_session' flag is not
> reset...
> Now, if the random session number from this endpoint is less than the
> previous one, all the RESET_MSGs from this endpoint will be rejected by
> the peer. In the other direction, when this link endpoint receives a
> RESET_MSG from the peer, it moves to ESTABLISHING and starts to send
> ACTIVATE_MSGs, but all these messages will be rejected by the peer too.
> As a result, the link cannot be re-established but gets stuck with this
> link endpoint in state ESTABLISHING and the peer in RESET!
> 
> Solution:
> ===
> 
> This link endpoint should not go directly to ESTABLISHED when getting
> ACTIVATE_MSG from the peer which may belong to the old session if the
> link was re-created. To ensure the session to be correct before the
> link is re-established, the peer endpoint in ESTABLISHING state will
> send back the last session number in ACTIVATE_MSG for a verification at
> this endpoint. Then, if needed, a new and more appropriate session
> number will be regenerated to force a re-synch first.
> 
> In addition, when a link in ESTABLISHING state is reset, its state will
> move to RESET according to the link FSM, along with resetting the
> 'in_session' flag (and the other data) as a normal link reset, it will
> also be deleted if requested.
> 
> The solution is backward compatible.
> 
> Signed-off-by: Tuong Lien 

Acked-by: Ying Xue 

> ---
>  net/tipc/link.c | 15 +++
>  net/tipc/msg.h  | 22 ++
>  net/tipc/node.c | 11 ++-
>  3 files changed, 43 insertions(+), 5 deletions(-)
> 
> diff --git a/net/tipc/link.c b/net/tipc/link.c
> index 2792a3cae682..ce3363d8f1c8 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -1425,6 +1425,10 @@ static void tipc_link_build_proto_msg(struct tipc_link 
> *l, int mtyp, bool probe,
>   l->rcv_unacked = 0;
>   } else {
>   /* RESET_MSG or ACTIVATE_MSG */
> + if (mtyp == ACTIVATE_MSG) {
> + msg_set_dest_session_valid(hdr, 1);
> + msg_set_dest_session(hdr, l->peer_session);
> + }
>   msg_set_max_pkt(hdr, l->advertised_mtu);
>   strcpy(data, l->if_name);
>   msg_set_size(hdr, INT_H_SIZE + TIPC_MAX_IF_NAME);
> @@ -1642,6 +1646,17 @@ static int tipc_link_proto_rcv(struct tipc_link *l, 
> struct sk_buff *skb,
>   rc = tipc_link_fsm_evt(l, LINK_FAILURE_EVT);
>   break;
>   }
> +
> + /* If this endpoint was re-created while peer was ESTABLISHING
> +  * it doesn't know current session number. Force re-synch.
&g

Re: [tipc-discussion] [net] tipc: fix skb may be leaky in tipc_link_input

[Ying Xue]

On 1/30/19 2:45 PM, Hoang Le wrote:
> When we free skb at tipc_data_input, we return a 'false' boolean.
> Then, skb passed to subcalling tipc_link_input in tipc_link_rcv,
> 
> 
> 1303 int tipc_link_rcv:
> ...
> 1354if (!tipc_data_input(l, skb, l->inputq))
> 1355rc |= tipc_link_input(l, skb, l->inputq);
> 
> 
> Fix it by simple changing to a 'true' boolean when skb is being free-ed.
> Then, tipc_link_rcv will bypassed to subcalling tipc_link_input as above
> condition.
> 
> Signed-off-by: Hoang Le 

Acked-by: Ying Xue 

> ---
>  net/tipc/link.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/tipc/link.c b/net/tipc/link.c
> index ac306d17f8ad..d31f83a9a2c5 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -1145,7 +1145,7 @@ static bool tipc_data_input(struct tipc_link *l, struct 
> sk_buff *skb,
>   default:
>   pr_warn("Dropping received illegal msg type\n");
>   kfree_skb(skb);
> - return false;
> + return true;
>   };
>  }
>  
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH] tipc: remove dead code in struct tipc_topsrv

[Ying Xue]

On 1/24/19 10:06 AM, Zhaolong Zhang wrote:
> max_rcvbuf_size is no longer used since commit "414574a0af36".
> 
> Signed-off-by: Zhaolong Zhang 

Acked-by: Ying Xue 

> ---
>  net/tipc/topsrv.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index efb16f6..4b1c083 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c
> @@ -60,7 +60,6 @@
>   * @awork: accept work item
>   * @rcv_wq: receive workqueue
>   * @send_wq: send workqueue
> - * @max_rcvbuf_size: maximum permitted receive message length
>   * @listener: topsrv listener socket
>   * @name: server name
>   */
> @@ -72,7 +71,6 @@ struct tipc_topsrv {
>   struct work_struct awork;
>   struct workqueue_struct *rcv_wq;
>   struct workqueue_struct *send_wq;
> - int max_rcvbuf_size;
>   struct socket *listener;
>   char name[TIPC_SERVER_NAME_LEN];
>  };
> @@ -648,7 +646,6 @@ int tipc_topsrv_start(struct net *net)
>   return -ENOMEM;
>  
>   srv->net = net;
> - srv->max_rcvbuf_size = sizeof(struct tipc_subscr);
>   INIT_WORK(>awork, tipc_topsrv_accept);
>  
>   strscpy(srv->name, name, sizeof(srv->name));
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [net 6/6] tipc: fix uninit-value in tipc_nl_compat_doit

[Ying Xue]

BUG: KMSAN: uninit-value in tipc_nl_compat_doit+0x404/0xa10 
net/tipc/netlink_compat.c:335
CPU: 0 PID: 4514 Comm: syz-executor485 Not tainted 4.16.0+ #87
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 
01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:17 [inline]
 dump_stack+0x185/0x1d0 lib/dump_stack.c:53
 kmsan_report+0x142/0x240 mm/kmsan/kmsan.c:1067
 __msan_warning_32+0x6c/0xb0 mm/kmsan/kmsan_instr.c:683
 tipc_nl_compat_doit+0x404/0xa10 net/tipc/netlink_compat.c:335
 tipc_nl_compat_recv+0x164b/0x2700 net/tipc/netlink_compat.c:1153
 genl_family_rcv_msg net/netlink/genetlink.c:599 [inline]
 genl_rcv_msg+0x1686/0x1810 net/netlink/genetlink.c:624
 netlink_rcv_skb+0x378/0x600 net/netlink/af_netlink.c:2447
 genl_rcv+0x63/0x80 net/netlink/genetlink.c:635
 netlink_unicast_kernel net/netlink/af_netlink.c:1311 [inline]
 netlink_unicast+0x166b/0x1740 net/netlink/af_netlink.c:1337
 netlink_sendmsg+0x1048/0x1310 net/netlink/af_netlink.c:1900
 sock_sendmsg_nosec net/socket.c:630 [inline]
 sock_sendmsg net/socket.c:640 [inline]
 ___sys_sendmsg+0xec0/0x1310 net/socket.c:2046
 __sys_sendmsg net/socket.c:2080 [inline]
 SYSC_sendmsg+0x2a3/0x3d0 net/socket.c:2091
 SyS_sendmsg+0x54/0x80 net/socket.c:2087
 do_syscall_64+0x309/0x430 arch/x86/entry/common.c:287
 entry_SYSCALL_64_after_hwframe+0x3d/0xa2
RIP: 0033:0x43fda9
RSP: 002b:7ffd0c184ba8 EFLAGS: 0213 ORIG_RAX: 002e
RAX: ffda RBX: 004002c8 RCX: 0043fda9
RDX:  RSI: 20023000 RDI: 0003
RBP: 006ca018 R08: 004002c8 R09: 004002c8
R10: 004002c8 R11: 0213 R12: 004016d0
R13: 00401760 R14:  R15: 

Uninit was created at:
 kmsan_save_stack_with_flags mm/kmsan/kmsan.c:278 [inline]
 kmsan_internal_poison_shadow+0xb8/0x1b0 mm/kmsan/kmsan.c:188
 kmsan_kmalloc+0x94/0x100 mm/kmsan/kmsan.c:314
 kmsan_slab_alloc+0x11/0x20 mm/kmsan/kmsan.c:321
 slab_post_alloc_hook mm/slab.h:445 [inline]
 slab_alloc_node mm/slub.c:2737 [inline]
 __kmalloc_node_track_caller+0xaed/0x11c0 mm/slub.c:4369
 __kmalloc_reserve net/core/skbuff.c:138 [inline]
 __alloc_skb+0x2cf/0x9f0 net/core/skbuff.c:206
 alloc_skb include/linux/skbuff.h:984 [inline]
 netlink_alloc_large_skb net/netlink/af_netlink.c:1183 [inline]
 netlink_sendmsg+0x9a6/0x1310 net/netlink/af_netlink.c:1875
 sock_sendmsg_nosec net/socket.c:630 [inline]
 sock_sendmsg net/socket.c:640 [inline]
 ___sys_sendmsg+0xec0/0x1310 net/socket.c:2046
 __sys_sendmsg net/socket.c:2080 [inline]
 SYSC_sendmsg+0x2a3/0x3d0 net/socket.c:2091
 SyS_sendmsg+0x54/0x80 net/socket.c:2087
 do_syscall_64+0x309/0x430 arch/x86/entry/common.c:287
 entry_SYSCALL_64_after_hwframe+0x3d/0xa2

In tipc_nl_compat_recv(), when the len variable returned by
nlmsg_attrlen() is 0, the message is still treated as a valid one,
which is obviously unresonable. When len is zero, it means the
message not only doesn't contain any valid TLV payload, but also
TLV header is not included. Under this stituation, tlv_type field
in TLV header is still accessed in tipc_nl_compat_dumpit() or
tipc_nl_compat_doit(), but the field space is obviously illegal.
Of course, it is not initialized.

Reported-by: syzbot+bca0dc46634781f08...@syzkaller.appspotmail.com
Reported-by: syzbot+6bdb590321a7ae40c...@syzkaller.appspotmail.com
Signed-off-by: Ying Xue 
---
 net/tipc/netlink_compat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index b90786c..4ad3586 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -1256,7 +1256,7 @@ static int tipc_nl_compat_recv(struct sk_buff *skb, 
struct genl_info *info)
}
 
len = nlmsg_attrlen(req_nlh, GENL_HDRLEN + TIPC_GENL_HDRLEN);
-   if (len && !TLV_OK(msg.req, len)) {
+   if (!len || !TLV_OK(msg.req, len)) {
msg.rep = tipc_get_err_tlv(TIPC_CFG_NOT_SUPPORTED);
err = -EOPNOTSUPP;
goto send;
-- 
2.7.4



___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [net 2/6] tipc: fix uninit-value in tipc_nl_compat_link_reset_stats

[Ying Xue]

syzbot reports following splat:

BUG: KMSAN: uninit-value in strlen+0x3b/0xa0 lib/string.c:486
CPU: 1 PID: 11057 Comm: syz-executor0 Not tainted 4.20.0-rc7+ #2
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 
01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x173/0x1d0 lib/dump_stack.c:113
 kmsan_report+0x12e/0x2a0 mm/kmsan/kmsan.c:613
 __msan_warning+0x82/0xf0 mm/kmsan/kmsan_instr.c:295
 strlen+0x3b/0xa0 lib/string.c:486
 nla_put_string include/net/netlink.h:1154 [inline]
 tipc_nl_compat_link_reset_stats+0x1f0/0x360 net/tipc/netlink_compat.c:760
 __tipc_nl_compat_doit net/tipc/netlink_compat.c:311 [inline]
 tipc_nl_compat_doit+0x3aa/0xaf0 net/tipc/netlink_compat.c:344
 tipc_nl_compat_handle net/tipc/netlink_compat.c:1107 [inline]
 tipc_nl_compat_recv+0x14d7/0x2760 net/tipc/netlink_compat.c:1210
 genl_family_rcv_msg net/netlink/genetlink.c:601 [inline]
 genl_rcv_msg+0x185f/0x1a60 net/netlink/genetlink.c:626
 netlink_rcv_skb+0x444/0x640 net/netlink/af_netlink.c:2477
 genl_rcv+0x63/0x80 net/netlink/genetlink.c:637
 netlink_unicast_kernel net/netlink/af_netlink.c:1310 [inline]
 netlink_unicast+0xf40/0x1020 net/netlink/af_netlink.c:1336
 netlink_sendmsg+0x127f/0x1300 net/netlink/af_netlink.c:1917
 sock_sendmsg_nosec net/socket.c:621 [inline]
 sock_sendmsg net/socket.c:631 [inline]
 ___sys_sendmsg+0xdb9/0x11b0 net/socket.c:2116
 __sys_sendmsg net/socket.c:2154 [inline]
 __do_sys_sendmsg net/socket.c:2163 [inline]
 __se_sys_sendmsg+0x305/0x460 net/socket.c:2161
 __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2161
 do_syscall_64+0xbc/0xf0 arch/x86/entry/common.c:291
 entry_SYSCALL_64_after_hwframe+0x63/0xe7
RIP: 0033:0x457ec9
Code: 6d b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 
89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 
3b b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:7f2557338c78 EFLAGS: 0246 ORIG_RAX: 002e
RAX: ffda RBX: 0003 RCX: 00457ec9
RDX:  RSI: 21c0 RDI: 0003
RBP: 0073bf00 R08:  R09: 
R10:  R11: 0246 R12: 7f25573396d4
R13: 004cb478 R14: 004d86c8 R15: 

Uninit was created at:
 kmsan_save_stack_with_flags mm/kmsan/kmsan.c:204 [inline]
 kmsan_internal_poison_shadow+0x92/0x150 mm/kmsan/kmsan.c:158
 kmsan_kmalloc+0xa6/0x130 mm/kmsan/kmsan_hooks.c:176
 kmsan_slab_alloc+0xe/0x10 mm/kmsan/kmsan_hooks.c:185
 slab_post_alloc_hook mm/slab.h:446 [inline]
 slab_alloc_node mm/slub.c:2759 [inline]
 __kmalloc_node_track_caller+0xe18/0x1030 mm/slub.c:4383
 __kmalloc_reserve net/core/skbuff.c:137 [inline]
 __alloc_skb+0x309/0xa20 net/core/skbuff.c:205
 alloc_skb include/linux/skbuff.h:998 [inline]
 netlink_alloc_large_skb net/netlink/af_netlink.c:1182 [inline]
 netlink_sendmsg+0xb82/0x1300 net/netlink/af_netlink.c:1892
 sock_sendmsg_nosec net/socket.c:621 [inline]
 sock_sendmsg net/socket.c:631 [inline]
 ___sys_sendmsg+0xdb9/0x11b0 net/socket.c:2116
 __sys_sendmsg net/socket.c:2154 [inline]
 __do_sys_sendmsg net/socket.c:2163 [inline]
 __se_sys_sendmsg+0x305/0x460 net/socket.c:2161
 __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2161
 do_syscall_64+0xbc/0xf0 arch/x86/entry/common.c:291
 entry_SYSCALL_64_after_hwframe+0x63/0xe7

The uninitialised access happened in tipc_nl_compat_link_reset_stats:
nla_put_string(skb, TIPC_NLA_LINK_NAME, name)

This is because name string is not validated before it's used.

Reported-by: syzbot+e01d94b5a4c266be6...@syzkaller.appspotmail.com
Signed-off-by: Ying Xue 
---
 net/tipc/netlink_compat.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index 77e4b24..b2b115b 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -87,6 +87,11 @@ static int tipc_skb_tailroom(struct sk_buff *skb)
return limit;
 }
 
+static inline int TLV_GET_DATA_LEN(struct tlv_desc *tlv)
+{
+   return TLV_GET_LEN(tlv) - TLV_SPACE(0);
+}
+
 static int tipc_add_tlv(struct sk_buff *skb, u16 type, void *data, u16 len)
 {
struct tlv_desc *tlv = (struct tlv_desc *)skb_tail_pointer(skb);
@@ -166,6 +171,11 @@ static struct sk_buff *tipc_get_err_tlv(char *str)
return buf;
 }
 
+static inline bool string_is_valid(char *s, int len)
+{
+   return memchr(s, '\0', len) ? true : false;
+}
+
 static int __tipc_nl_compat_dumpit(struct tipc_nl_compat_cmd_dump *cmd,
   struct tipc_nl_compat_msg *msg,
   struct sk_buff *arg)
@@ -750,6 +760,7 @@ static int tipc_nl_compat_link_reset_stats(struct 
tipc_nl_compat_cmd_doit *cmd,
 {
char *name;
struct nlattr *link;
+   int len;
 
name = (char *)TLV_DATA(msg->req);
 
@@ -757,6 +768,10 @@ static int tipc_nl_compat_link_reset_stats(struct 
tipc_nl_compat_cmd_

[tipc-discussion] [net 0/6] tipc: fix uninit-value issues reported by syzbot

[Ying Xue]

Recently, syzbot complained that TIPC module exits several issues
associated with uninit-value type. So, in this series, we try to
fix them as many as possible.

Ying Xue (6):
  tipc: fix uninit-value in in tipc_conn_rcv_sub
  tipc: fix uninit-value in tipc_nl_compat_link_reset_stats
  tipc: fix uninit-value in tipc_nl_compat_bearer_enable
  tipc: fix uninit-value in tipc_nl_compat_link_set
  tipc: fix uninit-value in tipc_nl_compat_name_table_dump
  tipc: fix uninit-value in tipc_nl_compat_doit

 net/tipc/netlink_compat.c | 50 ++-
 net/tipc/topsrv.c |  2 +-
 2 files changed, 50 insertions(+), 2 deletions(-)

-- 
2.7.4



___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [net 5/6] tipc: fix uninit-value in tipc_nl_compat_name_table_dump

[Ying Xue]

syzbot reported:

BUG: KMSAN: uninit-value in __arch_swab32 arch/x86/include/uapi/asm/swab.h:10 
[inline]
BUG: KMSAN: uninit-value in __fswab32 include/uapi/linux/swab.h:59 [inline]
BUG: KMSAN: uninit-value in tipc_nl_compat_name_table_dump+0x4a8/0xba0 
net/tipc/netlink_compat.c:826
CPU: 0 PID: 6290 Comm: syz-executor848 Not tainted 4.19.0-rc8+ #70
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 
01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x306/0x460 lib/dump_stack.c:113
 kmsan_report+0x1a2/0x2e0 mm/kmsan/kmsan.c:917
 __msan_warning+0x7c/0xe0 mm/kmsan/kmsan_instr.c:500
 __arch_swab32 arch/x86/include/uapi/asm/swab.h:10 [inline]
 __fswab32 include/uapi/linux/swab.h:59 [inline]
 tipc_nl_compat_name_table_dump+0x4a8/0xba0 net/tipc/netlink_compat.c:826
 __tipc_nl_compat_dumpit+0x59e/0xdb0 net/tipc/netlink_compat.c:205
 tipc_nl_compat_dumpit+0x63a/0x820 net/tipc/netlink_compat.c:270
 tipc_nl_compat_handle net/tipc/netlink_compat.c:1151 [inline]
 tipc_nl_compat_recv+0x1402/0x2760 net/tipc/netlink_compat.c:1210
 genl_family_rcv_msg net/netlink/genetlink.c:601 [inline]
 genl_rcv_msg+0x185c/0x1a20 net/netlink/genetlink.c:626
 netlink_rcv_skb+0x394/0x640 net/netlink/af_netlink.c:2454
 genl_rcv+0x63/0x80 net/netlink/genetlink.c:637
 netlink_unicast_kernel net/netlink/af_netlink.c:1317 [inline]
 netlink_unicast+0x166d/0x1720 net/netlink/af_netlink.c:1343
 netlink_sendmsg+0x1391/0x1420 net/netlink/af_netlink.c:1908
 sock_sendmsg_nosec net/socket.c:621 [inline]
 sock_sendmsg net/socket.c:631 [inline]
 ___sys_sendmsg+0xe47/0x1200 net/socket.c:2116
 __sys_sendmsg net/socket.c:2154 [inline]
 __do_sys_sendmsg net/socket.c:2163 [inline]
 __se_sys_sendmsg+0x307/0x460 net/socket.c:2161
 __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2161
 do_syscall_64+0xbe/0x100 arch/x86/entry/common.c:291
 entry_SYSCALL_64_after_hwframe+0x63/0xe7
RIP: 0033:0x440179
Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7 48 
89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 
fb 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:7ffecec49318 EFLAGS: 0213 ORIG_RAX: 002e
RAX: ffda RBX: 004002c8 RCX: 00440179
RDX:  RSI: 2100 RDI: 0003
RBP: 006ca018 R08:  R09: 004002c8
R10:  R11: 0213 R12: 00401a00
R13: 00401a90 R14:  R15: 

Uninit was created at:
 kmsan_save_stack_with_flags mm/kmsan/kmsan.c:255 [inline]
 kmsan_internal_poison_shadow+0xc8/0x1d0 mm/kmsan/kmsan.c:180
 kmsan_kmalloc+0xa4/0x120 mm/kmsan/kmsan_hooks.c:104
 kmsan_slab_alloc+0x10/0x20 mm/kmsan/kmsan_hooks.c:113
 slab_post_alloc_hook mm/slab.h:446 [inline]
 slab_alloc_node mm/slub.c:2727 [inline]
 __kmalloc_node_track_caller+0xb43/0x1400 mm/slub.c:4360
 __kmalloc_reserve net/core/skbuff.c:138 [inline]
 __alloc_skb+0x422/0xe90 net/core/skbuff.c:206
 alloc_skb include/linux/skbuff.h:996 [inline]
 netlink_alloc_large_skb net/netlink/af_netlink.c:1189 [inline]
 netlink_sendmsg+0xcaf/0x1420 net/netlink/af_netlink.c:1883
 sock_sendmsg_nosec net/socket.c:621 [inline]
 sock_sendmsg net/socket.c:631 [inline]
 ___sys_sendmsg+0xe47/0x1200 net/socket.c:2116
 __sys_sendmsg net/socket.c:2154 [inline]
 __do_sys_sendmsg net/socket.c:2163 [inline]
 __se_sys_sendmsg+0x307/0x460 net/socket.c:2161
 __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2161
 do_syscall_64+0xbe/0x100 arch/x86/entry/common.c:291
 entry_SYSCALL_64_after_hwframe+0x63/0xe7

We cannot take for granted the thing that the length of data contained
in TLV is longer than the size of struct tipc_name_table_query in
tipc_nl_compat_name_table_dump().

Reported-by: syzbot+06e771a754829716a...@syzkaller.appspotmail.com
Signed-off-by: Ying Xue 
---
 net/tipc/netlink_compat.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index 89e6ae3..b90786c 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -824,6 +824,8 @@ static int tipc_nl_compat_name_table_dump_header(struct 
tipc_nl_compat_msg *msg)
};
 
ntq = (struct tipc_name_table_query *)TLV_DATA(msg->req);
+   if (TLV_GET_DATA_LEN(msg->req) < sizeof(struct tipc_name_table_query))
+   return -EINVAL;
 
depth = ntohl(ntq->depth);
 
-- 
2.7.4



___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [net 1/6] tipc: fix uninit-value in in tipc_conn_rcv_sub

[Ying Xue]

syzbot reported:

BUG: KMSAN: uninit-value in tipc_conn_rcv_sub+0x184/0x950 net/tipc/topsrv.c:373
CPU: 0 PID: 66 Comm: kworker/u4:4 Not tainted 4.17.0-rc3+ #88
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 
01/01/2011
Workqueue: tipc_rcv tipc_conn_recv_work
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x185/0x1d0 lib/dump_stack.c:113
 kmsan_report+0x142/0x240 mm/kmsan/kmsan.c:1067
 __msan_warning_32+0x6c/0xb0 mm/kmsan/kmsan_instr.c:683
 tipc_conn_rcv_sub+0x184/0x950 net/tipc/topsrv.c:373
 tipc_conn_rcv_from_sock net/tipc/topsrv.c:409 [inline]
 tipc_conn_recv_work+0x3cd/0x560 net/tipc/topsrv.c:424
 process_one_work+0x12c6/0x1f60 kernel/workqueue.c:2145
 worker_thread+0x113c/0x24f0 kernel/workqueue.c:2279
 kthread+0x539/0x720 kernel/kthread.c:239
 ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:412

Local variable description: s.i@tipc_conn_recv_work
Variable was created at:
 tipc_conn_recv_work+0x65/0x560 net/tipc/topsrv.c:419
 process_one_work+0x12c6/0x1f60 kernel/workqueue.c:2145

In tipc_conn_rcv_from_sock(), it always supposes the length of message
received from sock_recvmsg() is not smaller than the size of struct
tipc_subscr. However, this assumption is false. Especially when the
length of received message is shorter than struct tipc_subscr size,
we will end up touching uninitialized fields in tipc_conn_rcv_sub().

Reported-by: syzbot+8951a3065ee7fd6d6...@syzkaller.appspotmail.com
Reported-by: syzbot+75e6e042c5bbf691f...@syzkaller.appspotmail.com
Signed-off-by: Ying Xue 
---
 net/tipc/topsrv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index efb16f6..a457c0f 100644
--- a/net/tipc/topsrv.c
+++ b/net/tipc/topsrv.c
@@ -398,7 +398,7 @@ static int tipc_conn_rcv_from_sock(struct tipc_conn *con)
ret = sock_recvmsg(con->sock, , MSG_DONTWAIT);
if (ret == -EWOULDBLOCK)
return -EWOULDBLOCK;
-   if (ret > 0) {
+   if (ret == sizeof(s)) {
read_lock_bh(>sk_callback_lock);
ret = tipc_conn_rcv_sub(srv, con, );
read_unlock_bh(>sk_callback_lock);
-- 
2.7.4



___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [net 4/6] tipc: fix uninit-value in tipc_nl_compat_link_set

[Ying Xue]

syzbot reports following splat:

BUG: KMSAN: uninit-value in strlen+0x3b/0xa0 lib/string.c:486
CPU: 1 PID: 9306 Comm: syz-executor172 Not tainted 4.20.0-rc7+ #2
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x173/0x1d0 lib/dump_stack.c:113
  kmsan_report+0x12e/0x2a0 mm/kmsan/kmsan.c:613
  __msan_warning+0x82/0xf0 mm/kmsan/kmsan_instr.c:313
  strlen+0x3b/0xa0 lib/string.c:486
  nla_put_string include/net/netlink.h:1154 [inline]
  __tipc_nl_compat_link_set net/tipc/netlink_compat.c:708 [inline]
  tipc_nl_compat_link_set+0x929/0x1220 net/tipc/netlink_compat.c:744
  __tipc_nl_compat_doit net/tipc/netlink_compat.c:311 [inline]
  tipc_nl_compat_doit+0x3aa/0xaf0 net/tipc/netlink_compat.c:344
  tipc_nl_compat_handle net/tipc/netlink_compat.c:1107 [inline]
  tipc_nl_compat_recv+0x14d7/0x2760 net/tipc/netlink_compat.c:1210
  genl_family_rcv_msg net/netlink/genetlink.c:601 [inline]
  genl_rcv_msg+0x185f/0x1a60 net/netlink/genetlink.c:626
  netlink_rcv_skb+0x444/0x640 net/netlink/af_netlink.c:2477
  genl_rcv+0x63/0x80 net/netlink/genetlink.c:637
  netlink_unicast_kernel net/netlink/af_netlink.c:1310 [inline]
  netlink_unicast+0xf40/0x1020 net/netlink/af_netlink.c:1336
  netlink_sendmsg+0x127f/0x1300 net/netlink/af_netlink.c:1917
  sock_sendmsg_nosec net/socket.c:621 [inline]
  sock_sendmsg net/socket.c:631 [inline]
  ___sys_sendmsg+0xdb9/0x11b0 net/socket.c:2116
  __sys_sendmsg net/socket.c:2154 [inline]
  __do_sys_sendmsg net/socket.c:2163 [inline]
  __se_sys_sendmsg+0x305/0x460 net/socket.c:2161
  __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2161
  do_syscall_64+0xbc/0xf0 arch/x86/entry/common.c:291
  entry_SYSCALL_64_after_hwframe+0x63/0xe7

The uninitialised access happened in
nla_put_string(skb, TIPC_NLA_LINK_NAME, lc->name)

This is because lc->name string is not validated before it's used.

Reported-by: syzbot+d78b8a29241a195ae...@syzkaller.appspotmail.com
Signed-off-by: Ying Xue 
---
 net/tipc/netlink_compat.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index 68a0b73..89e6ae3 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -762,9 +762,14 @@ static int tipc_nl_compat_link_set(struct 
tipc_nl_compat_cmd_doit *cmd,
struct tipc_link_config *lc;
struct tipc_bearer *bearer;
struct tipc_media *media;
+   int len;
 
lc = (struct tipc_link_config *)TLV_DATA(msg->req);
 
+   len = min_t(int, TLV_GET_DATA_LEN(msg->req), TIPC_MAX_LINK_NAME);
+   if (!string_is_valid(lc->name, len))
+   return -EINVAL;
+
media = tipc_media_find(lc->name);
if (media) {
cmd->doit = &__tipc_nl_media_set;
-- 
2.7.4



___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [net 3/6] tipc: fix uninit-value in tipc_nl_compat_bearer_enable

[Ying Xue]

syzbot reported:

BUG: KMSAN: uninit-value in strlen+0x3b/0xa0 lib/string.c:484
CPU: 1 PID: 6371 Comm: syz-executor652 Not tainted 4.19.0-rc8+ #70
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 
01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x306/0x460 lib/dump_stack.c:113
 kmsan_report+0x1a2/0x2e0 mm/kmsan/kmsan.c:917
 __msan_warning+0x7c/0xe0 mm/kmsan/kmsan_instr.c:500
 strlen+0x3b/0xa0 lib/string.c:484
 nla_put_string include/net/netlink.h:1011 [inline]
 tipc_nl_compat_bearer_enable+0x238/0x7b0 net/tipc/netlink_compat.c:389
 __tipc_nl_compat_doit net/tipc/netlink_compat.c:311 [inline]
 tipc_nl_compat_doit+0x39f/0xae0 net/tipc/netlink_compat.c:344
 tipc_nl_compat_recv+0x147c/0x2760 net/tipc/netlink_compat.c:1107
 genl_family_rcv_msg net/netlink/genetlink.c:601 [inline]
 genl_rcv_msg+0x185c/0x1a20 net/netlink/genetlink.c:626
 netlink_rcv_skb+0x394/0x640 net/netlink/af_netlink.c:2454
 genl_rcv+0x63/0x80 net/netlink/genetlink.c:637
 netlink_unicast_kernel net/netlink/af_netlink.c:1317 [inline]
 netlink_unicast+0x166d/0x1720 net/netlink/af_netlink.c:1343
 netlink_sendmsg+0x1391/0x1420 net/netlink/af_netlink.c:1908
 sock_sendmsg_nosec net/socket.c:621 [inline]
 sock_sendmsg net/socket.c:631 [inline]
 ___sys_sendmsg+0xe47/0x1200 net/socket.c:2116
 __sys_sendmsg net/socket.c:2154 [inline]
 __do_sys_sendmsg net/socket.c:2163 [inline]
 __se_sys_sendmsg+0x307/0x460 net/socket.c:2161
 __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2161
 do_syscall_64+0xbe/0x100 arch/x86/entry/common.c:291
 entry_SYSCALL_64_after_hwframe+0x63/0xe7
RIP: 0033:0x440179
Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7 48 
89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 
fb 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:7fffef7beee8 EFLAGS: 0213 ORIG_RAX: 002e
RAX: ffda RBX: 004002c8 RCX: 00440179
RDX:  RSI: 2100 RDI: 0003
RBP: 006ca018 R08:  R09: 004002c8
R10:  R11: 0213 R12: 00401a00
R13: 00401a90 R14:  R15: 

Uninit was created at:
 kmsan_save_stack_with_flags mm/kmsan/kmsan.c:255 [inline]
 kmsan_internal_poison_shadow+0xc8/0x1d0 mm/kmsan/kmsan.c:180
 kmsan_kmalloc+0xa4/0x120 mm/kmsan/kmsan_hooks.c:104
 kmsan_slab_alloc+0x10/0x20 mm/kmsan/kmsan_hooks.c:113
 slab_post_alloc_hook mm/slab.h:446 [inline]
 slab_alloc_node mm/slub.c:2727 [inline]
 __kmalloc_node_track_caller+0xb43/0x1400 mm/slub.c:4360
 __kmalloc_reserve net/core/skbuff.c:138 [inline]
 __alloc_skb+0x422/0xe90 net/core/skbuff.c:206
 alloc_skb include/linux/skbuff.h:996 [inline]
 netlink_alloc_large_skb net/netlink/af_netlink.c:1189 [inline]
 netlink_sendmsg+0xcaf/0x1420 net/netlink/af_netlink.c:1883
 sock_sendmsg_nosec net/socket.c:621 [inline]
 sock_sendmsg net/socket.c:631 [inline]
 ___sys_sendmsg+0xe47/0x1200 net/socket.c:2116
 __sys_sendmsg net/socket.c:2154 [inline]
 __do_sys_sendmsg net/socket.c:2163 [inline]
 __se_sys_sendmsg+0x307/0x460 net/socket.c:2161
 __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2161
 do_syscall_64+0xbe/0x100 arch/x86/entry/common.c:291
 entry_SYSCALL_64_after_hwframe+0x63/0xe7

The root cause is that we don't validate whether bear name is a valid
string in tipc_nl_compat_bearer_enable().

Meanwhile, we also fix the same issue in the following functions:
tipc_nl_compat_bearer_disable()
tipc_nl_compat_link_stat_dump()
tipc_nl_compat_media_set()
tipc_nl_compat_bearer_set()

Reported-by: syzbot+b33d5cae0efd35dbf...@syzkaller.appspotmail.com
Signed-off-by: Ying Xue 
---
 net/tipc/netlink_compat.c | 26 ++
 1 file changed, 26 insertions(+)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index b2b115b..68a0b73 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -389,6 +389,7 @@ static int tipc_nl_compat_bearer_enable(struct 
tipc_nl_compat_cmd_doit *cmd,
struct nlattr *prop;
struct nlattr *bearer;
struct tipc_bearer_config *b;
+   int len;
 
b = (struct tipc_bearer_config *)TLV_DATA(msg->req);
 
@@ -396,6 +397,10 @@ static int tipc_nl_compat_bearer_enable(struct 
tipc_nl_compat_cmd_doit *cmd,
if (!bearer)
return -EMSGSIZE;
 
+   len = min_t(int, TLV_GET_DATA_LEN(msg->req), TIPC_MAX_BEARER_NAME);
+   if (!string_is_valid(b->name, len))
+   return -EINVAL;
+
if (nla_put_string(skb, TIPC_NLA_BEARER_NAME, b->name))
return -EMSGSIZE;
 
@@ -421,6 +426,7 @@ static int tipc_nl_compat_bearer_disable(struct 
tipc_nl_compat_cmd_doit *cmd,
 {
char *name;
struct nlattr *bearer;
+   int len;
 
name = (char *)TLV_DATA(msg->req);
 
@@ -428,6 +434,10 @@ static int tipc_nl_compat_bearer_disable(struct 
tipc_nl_compat_cmd_doit *cmd

Re: [tipc-discussion] [PATCH net] tipc: fix uninit-value in tipc_nl_compat_link_set

[Ying Xue]

On 1/7/19 9:38 PM, David Miller wrote:
> From: Ying Xue 
> Date: Mon, 7 Jan 2019 19:29:52 +0800
> 
>> This is because lc->name string is not validated before it's used.
> 
> It looks like we have several situations like this, not just this one.
> 
> For example, tipc_nl_compat_bearer_{enable,disable}() with b->name.
> 
> Next, tipc_nl_compat_media_set() and tipc_nl_compat_bearer_set().
> 
> On input, tipc_nl_compat_link_stat_dump() blindly does a strcmp()
> on one of these strings.
> 
> In fact, this entire file is full of errors of this sort.
> 
> Can you please address all of them, perhaps using a helper of
> some kind to consolidate the logic?
> 

Thank you for your good suggestions. I will solve them as soon as possible.

Regards,
Ying

> Thank you.
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [PATCH net] tipc: fix uninit-value in tipc_nl_compat_link_set

[Ying Xue]

syzbot reports following splat:

BUG: KMSAN: uninit-value in strlen+0x3b/0xa0 lib/string.c:486
CPU: 1 PID: 9306 Comm: syz-executor172 Not tainted 4.20.0-rc7+ #2
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x173/0x1d0 lib/dump_stack.c:113
  kmsan_report+0x12e/0x2a0 mm/kmsan/kmsan.c:613
  __msan_warning+0x82/0xf0 mm/kmsan/kmsan_instr.c:313
  strlen+0x3b/0xa0 lib/string.c:486
  nla_put_string include/net/netlink.h:1154 [inline]
  __tipc_nl_compat_link_set net/tipc/netlink_compat.c:708 [inline]
  tipc_nl_compat_link_set+0x929/0x1220 net/tipc/netlink_compat.c:744
  __tipc_nl_compat_doit net/tipc/netlink_compat.c:311 [inline]
  tipc_nl_compat_doit+0x3aa/0xaf0 net/tipc/netlink_compat.c:344
  tipc_nl_compat_handle net/tipc/netlink_compat.c:1107 [inline]
  tipc_nl_compat_recv+0x14d7/0x2760 net/tipc/netlink_compat.c:1210
  genl_family_rcv_msg net/netlink/genetlink.c:601 [inline]
  genl_rcv_msg+0x185f/0x1a60 net/netlink/genetlink.c:626
  netlink_rcv_skb+0x444/0x640 net/netlink/af_netlink.c:2477
  genl_rcv+0x63/0x80 net/netlink/genetlink.c:637
  netlink_unicast_kernel net/netlink/af_netlink.c:1310 [inline]
  netlink_unicast+0xf40/0x1020 net/netlink/af_netlink.c:1336
  netlink_sendmsg+0x127f/0x1300 net/netlink/af_netlink.c:1917
  sock_sendmsg_nosec net/socket.c:621 [inline]
  sock_sendmsg net/socket.c:631 [inline]
  ___sys_sendmsg+0xdb9/0x11b0 net/socket.c:2116
  __sys_sendmsg net/socket.c:2154 [inline]
  __do_sys_sendmsg net/socket.c:2163 [inline]
  __se_sys_sendmsg+0x305/0x460 net/socket.c:2161
  __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2161
  do_syscall_64+0xbc/0xf0 arch/x86/entry/common.c:291
  entry_SYSCALL_64_after_hwframe+0x63/0xe7

The uninitialised access happened in
nla_put_string(skb, TIPC_NLA_LINK_NAME, lc->name)

This is because lc->name string is not validated before it's used.

Reported-by: syzbot+d78b8a29241a195ae...@syzkaller.appspotmail.com
Signed-off-by: Ying Xue 
---
 net/tipc/netlink_compat.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index 21f6ccc..bbf3f5a 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -705,6 +705,9 @@ static int __tipc_nl_compat_link_set(struct sk_buff *skb,
if (!link)
return -EMSGSIZE;
 
+   if (!memchr(lc->name, '\0', TIPC_MAX_LINK_NAME))
+   return -EINVAL;
+
if (nla_put_string(skb, TIPC_NLA_LINK_NAME, lc->name))
return -EMSGSIZE;
 
-- 
2.7.4



___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH] tipc: fix memory leak in tipc_nl_compat_publ_dump

[Ying Xue]

On 1/6/19 12:52 AM, Gustavo A. R. Silva wrote:
> There is a memory leak in case genlmsg_put fails.
> 
> Fix this by freeing *args* before return.
> 
> Addresses-Coverity-ID: 1476406 ("Resource leak")
> Fixes: 46273cf7e009 ("tipc: fix a missing check of genlmsg_put")
> Signed-off-by: Gustavo A. R. Silva 

Acked-by: Ying Xue 

> ---
>  net/tipc/netlink_compat.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
> index 40f5cae623a7..77e4b2418f30 100644
> --- a/net/tipc/netlink_compat.c
> +++ b/net/tipc/netlink_compat.c
> @@ -904,8 +904,10 @@ static int tipc_nl_compat_publ_dump(struct 
> tipc_nl_compat_msg *msg, u32 sock)
>  
>   hdr = genlmsg_put(args, 0, 0, _genl_family, NLM_F_MULTI,
> TIPC_NL_PUBL_GET);
> - if (!hdr)
> + if (!hdr) {
> + kfree_skb(args);
>   return -EMSGSIZE;
> + }
>  
>   nest = nla_nest_start(args, TIPC_NLA_SOCK);
>   if (!nest) {
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net-next] tipc: fix uninitialized value for broadcast retransmission

[Ying Xue]

Good catch!

On 12/10/18 12:08 PM, Hoang Le wrote:
> When sending broadcast message on high load system, there are a lot of
> unnecessary packets restranmission. That issue was caused by missing in
> initial criteria for retransmission.
> 
> To prevent this happen, just initialize this criteria for retransmission
> in next 10 milliseconds.
> 
> Fixes: 31c4f4cc32f7 tipc: improve broadcast retransmission algorithm
> Acked-by: Jon Maloy 
> Signed-off-by: Hoang Le 

Acked-by: Ying Xue 

> ---
>  net/tipc/link.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/net/tipc/link.c b/net/tipc/link.c
> index 9e265eb89726..6ffa43819727 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -945,6 +945,10 @@ int tipc_link_xmit(struct tipc_link *l, struct 
> sk_buff_head *list,
>   }
>   __skb_dequeue(list);
>   __skb_queue_tail(transmq, skb);
> + /* next retransmit attempt */
> + if (link_is_bc_sndlink(l))
> + TIPC_SKB_CB(skb)->nxt_retr =
> + jiffies + TIPC_BC_RETR_LIM;
>   __skb_queue_tail(xmitq, _skb);
>   TIPC_SKB_CB(skb)->ackers = l->ackers;
>   l->rcv_unacked = 0;
> @@ -992,6 +996,9 @@ static void tipc_link_advance_backlog(struct tipc_link *l,
>   hdr = buf_msg(skb);
>   l->backlog[msg_importance(hdr)].len--;
>   __skb_queue_tail(>transmq, skb);
> + /* next retransmit attempt */
> + if (link_is_bc_sndlink(l))
> + TIPC_SKB_CB(skb)->nxt_retr = jiffies + TIPC_BC_RETR_LIM;
>   __skb_queue_tail(xmitq, _skb);
>   TIPC_SKB_CB(skb)->ackers = l->ackers;
>   msg_set_seqno(hdr, seqno);
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH v2 0/5] tipc: tracepoints and trace_events in TIPC

[Ying Xue]

Your series is good to me.

Acked-by: Ying Xue 

On 12/18/18 6:11 PM, Tuong Lien wrote:
> The patch series is the first step of introducing a tracing framework in
> TIPC, which will assist in collecting complete & plentiful data for post
> analysis, even in the case of a single failure occurrence e.g. when the
> failure is unreproducible.
> 
> The tracing code in TIPC utilizes the powerful kernel tracepoints, trace
> events features along with particular dump functions to trace the TIPC
> object data and events (incl. bearer, link, socket, node, etc.).
> 
> The tracing code should generate zero-load to TIPC when the trace events
> are not enabled.
> 
> v2: improve trace output format;
> improve the socket traces, make it now convenient for user to do the
> socket traces on any specific socket when needed (by using the new
> sysctl file - '/proc/sys/net/tipc/sk_filter' with a socket 'tuple'
> for filtering);
> add a new dump option for link's wakeup queue, add trace for link
> congestion situation;
> improve the node and bearer traces;
> 
> Tuong Lien (5):
>   tipc: enable tracepoints in tipc
>   tipc: add trace_events for tipc link
>   tipc: add trace_events for tipc socket
>   tipc: add trace_events for tipc node
>   tipc: add trace_events for tipc bearer
> 
>  net/tipc/Makefile |   4 +-
>  net/tipc/bearer.c |   9 +-
>  net/tipc/bearer.h |   2 +-
>  net/tipc/link.c   | 153 ++-
>  net/tipc/link.h   |   2 +
>  net/tipc/node.c   |  88 ++-
>  net/tipc/node.h   |   1 +
>  net/tipc/socket.c | 230 -
>  net/tipc/socket.h |   4 +
>  net/tipc/sysctl.c |   8 +
>  net/tipc/trace.c  | 206 ++
>  net/tipc/trace.h  | 431 
> ++
>  12 files changed, 1122 insertions(+), 16 deletions(-)
>  create mode 100644 net/tipc/trace.c
>  create mode 100644 net/tipc/trace.h
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH v1 1/5] tipc: enable tracepoints in tipc

[Ying Xue]

On 12/10/18 12:08 PM, Tuong Lien Tong wrote:
> Thank you very much for having reviewed and tested the patch!
> Regarding your observation about the "X" in the 'tipc/enable' file, it 
> usually happens when we just enable some of the TIPC trace events (or enable 
> all of them but then disable some...), so please double-check if this is 
> correct.

Yes, you are right. Good job! The series of patches is fine to me now.


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH v1 1/5] tipc: enable tracepoints in tipc

[Ying Xue]

Great job!

On 11/27/18 8:37 PM, Tuong Lien wrote:
> cat trace_pipe > /trace.out &
> 
> To disable all the TIPC trace_events:
> 
> echo 0 > /sys/kernel/debug/tracing/events/tipc/enable
> 
> To clear the trace buffer:
> 
> echo > trace
> 
> d) Like the other trace_events, the feature like 'filter' or 'trigger'
> is also usable for the tipc trace_events.
> For more details, have a look at:
> 
> Documentation/trace/ftrace.txt
> 
> MAINTAINERS | add two new files 'trace.h' & 'trace.c' in tipc
> 
> Signed-off-by: Tuong Lien 

Acked-by: Ying Xue 
Tested-by: Ying Xue 

The only minor thing is that I found the default value of
/sys/kernel/debug/tracing/events/tipc/enable is not 0, but it's "X".
Please check why.

> ---
>  net/tipc/Makefile |   4 +-
>  net/tipc/link.c   | 115 +++
>  net/tipc/link.h   |   1 +
>  net/tipc/node.c   |  61 +++


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net-next 2/2] tipc: fix lockdep warning during node delete

[Ying Xue]

Hi Jon,

I believe the real deadlock scenario is as below:

CPU0CPU1
   ===
spin_lock_bh(>node_list_lock);
 
 call_timer_fn()
 

   irq_fn()
 
del_timer_sync()
//wait until tipc_node_timeout() is completed
 

 tipc_node_timeout()
 tipc_node_cleanup()
 spin_lock_bh(>node_list_lock);

tn->node_list_lock has been held, but del_timer_sync() must wait until
tipc_node_timeout() is completed on other CPUs, otherwise, it will never
return. Exactly speaking, if tipc_node_timeout() is not finished,
del_timer_sync() could not exit from the loop below:

del_timer_sync()
for (;;) {
int ret = try_to_del_timer_sync(timer);
if (ret >= 0)
return ret;
cpu_relax();
}

However, tipc_node_timeout() will try to grab the tn->node_list_lock on
CPU1, but the lock has been taken before del_timer_sync() on CPU0. As a
result, system will be locked forever.

Regarding the root cause, as long as we don't synchronously wait the
event of tipc_node_timeout() completion on other CPUs, the deadlock will
never happen. It means if we replace del_timer_sync() with del_timer()
in tipc_node_delete(), the deadlock will be eliminated. But we need to
ensure node instance is valid especially for tipc_node_timeout() after
tipc_node_put() is called in tipc_node_delete(). If so, we need to
carefully increase and decrease node reference count.

In your solution, tn->node_list_lock would not be held in
tipc_node_timeout(), instead, grabbing the lock is moved to workqueue,
which means the deadlock is able to be killed too.

If we use the solution I suggested above, it will be a bit natural and
we don't need to additionally introduce workqueue. But as I said, we
have to carefully check whether we properly get/put node reference count.

Actually, we original used del_timer() to asynchronously delete node
timer, but as some issues were found, later we changed as its deletion
behavior with a synchronous manner.

Anyway, your solution is also fine to me. So you can decide which
approach you prefer to.

Thanks,
Ying

On 11/11/2018 03:05 AM, Jon Maloy wrote:
> We see the following lockdep warning:
> 
> [ 2284.078521] ==
> [ 2284.078604] WARNING: possible circular locking dependency detected
> [ 2284.078604] 4.19.0+ #42 Tainted: GE
> [ 2284.078604] --
> [ 2284.078604] rmmod/254 is trying to acquire lock:
> [ 2284.078604] acd94e28 ((>timer)#2){+.-.}, at: 
> del_timer_sync+0x5/0xa0
> [ 2284.078604]
> [ 2284.078604] but task is already holding lock:
> [ 2284.078604] f997afc0 (&(>node_list_lock)->rlock){+.-.}, at: 
> tipc_node_stop+0xac/0x190 [tipc]
> [ 2284.078604]
> [ 2284.078604] which lock already depends on the new lock.
> [ 2284.078604]
> [ 2284.078604]
> [ 2284.078604] the existing dependency chain (in reverse order) is:
> [ 2284.078604]
> [ 2284.078604] -> #1 (&(>node_list_lock)->rlock){+.-.}:
> [ 2284.078604]tipc_node_timeout+0x20a/0x330 [tipc]
> [ 2284.078604]call_timer_fn+0xa1/0x280
> [ 2284.078604]run_timer_softirq+0x1f2/0x4d0
> [ 2284.078604]__do_softirq+0xfc/0x413
> [ 2284.078604]irq_exit+0xb5/0xc0
> [ 2284.078604]smp_apic_timer_interrupt+0xac/0x210
> [ 2284.078604]apic_timer_interrupt+0xf/0x20
> [ 2284.078604]default_idle+0x1c/0x140
> [ 2284.078604]do_idle+0x1bc/0x280
> [ 2284.078604]cpu_startup_entry+0x19/0x20
> [ 2284.078604]start_secondary+0x187/0x1c0
> [ 2284.078604]secondary_startup_64+0xa4/0xb0
> [ 2284.078604]
> [ 2284.078604] -> #0 ((>timer)#2){+.-.}:
> [ 2284.078604]del_timer_sync+0x34/0xa0
> [ 2284.078604]tipc_node_delete+0x1a/0x40 [tipc]
> [ 2284.078604]tipc_node_stop+0xcb/0x190 [tipc]
> [ 2284.078604]tipc_net_stop+0x154/0x170 [tipc]
> [ 2284.078604]tipc_exit_net+0x16/0x30 [tipc]
> [ 2284.078604]ops_exit_list.isra.8+0x36/0x70
> [ 2284.078604]unregister_pernet_operations+0x87/0xd0
> [ 2284.078604]unregister_pernet_subsys+0x1d/0x30
> [ 2284.078604]tipc_exit+0x11/0x6f2 [tipc]
> [ 2284.078604]__x64_sys_delete_module+0x1df/0x240
> [ 2284.078604]do_syscall_64+0x66/0x460
> [ 2284.078604]entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 2284.078604]
> [ 2284.078604] other info that might help us debug this:
> [ 2284.078604]
> [ 2284.078604]  Possible unsafe locking scenario:
> [ 2284.078604]
> [ 2284.078604]CPU0CPU1
> [ 

Re: [tipc-discussion] [net-next 1/2] tipc: fix lockdep warning when reinitilaizing sockets

[Ying Xue]

On 11/11/2018 03:05 AM, Jon Maloy wrote:
>  
> +struct tipc_fin_net_work {
> + struct work_struct work;
> + struct net *net;
> + u32 addr;
> +};
> +

In my opinion, it's no necessary to define the name so specific to
reflect that it will be only used to finalize tipc net.

Instead, probably we can define a bit generic name: such as, tipc_net_work.


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net-next 1/2] tipc: fix lockdep warning when reinitilaizing sockets

[Ying Xue]

On 11/11/2018 03:05 AM, Jon Maloy wrote:
> + INIT_WORK(>work, tipc_exec_net_finalize);

The function name of tipc_exec_net_finalize doesn't match the
traditional naming rule about work function. It's better that it can be
named as tipc_net_finalize_work or tipc_net_finalize_fn.

Other changes are fine to me.

Acked-by: Ying Xue 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] Deadlock warning

[Ying Xue]

ipc]
> [   47.938117]  ? call_timer_fn+0x5/0x280
> [   47.938117]  ? tipc_disc_msg_xmit.isra.19+0xa0/0xa0 [tipc]
> [   47.938117]  ? tipc_disc_msg_xmit.isra.19+0xa0/0xa0 [tipc]
> [   47.938117]  call_timer_fn+0xa1/0x280
> [   47.938117]  ? tipc_disc_msg_xmit.isra.19+0xa0/0xa0 [tipc]
> [   47.938117]  run_timer_softirq+0x1f2/0x4d0
> [   47.938117]  __do_softirq+0xfc/0x413
> [   47.938117]  irq_exit+0xb5/0xc0
> [   47.938117]  smp_apic_timer_interrupt+0xac/0x210
> [   47.938117]  apic_timer_interrupt+0xf/0x20
> [   47.938117]  
> [   47.938117] RIP: 0010:default_idle+0x1c/0x140
> [   47.938117] Code: 90 90 90 90 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 
> 41 54 55 53 65 8b 2d d8 2b 74 65 0f 1f 44 00 00 e8 c6 2c 8b ff fb f4 <65> 8b 
> 2d c5 2b 74 65 0f 1f 44 00 00 5b 5d 41 5c c3 65 8b 05 b4 2b
> [   47.938117] RSP: 0018:af6ac0207ec8 EFLAGS: 0206 ORIG_RAX: 
> ff13
> [   47.938117] RAX: 8f5b3735e200 RBX: 0003 RCX: 
> 0001
> [   47.938117] RDX: 0001 RSI: 0001 RDI: 
> 8f5b3735e200
> [   47.938117] RBP: 0003 R08: 0001 R09: 
> 
> [   47.938117] R10:  R11:  R12: 
> 
> [   47.938117] R13:  R14: 8f5b3735e200 R15: 
> 8f5b3735e200
> [   47.938117]  ? default_idle+0x1a/0x140
> [   47.938117]  do_idle+0x1bc/0x280
> [   47.938117]  cpu_startup_entry+0x19/0x20
> [   47.938117]  start_secondary+0x187/0x1c0
> [   47.938117]  secondary_startup_64+0xa4/0xb0
> 
> The reason seems to be that tipc_net_finalize()->tipc_sk_reinit() is
> calling the function rhashtable_walk_enter() within a timer interrupt
> with dicoverer::lock set. It is safe to release this lock before we call
> tipc_net_finalize(), so that is what we do.
> 
> Signed-off-by: Jon Maloy 
> ---
>  net/tipc/discover.c | 17 +
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/net/tipc/discover.c b/net/tipc/discover.c
> index 2830709..5fff91c 100644
> --- a/net/tipc/discover.c
> +++ b/net/tipc/discover.c
> @@ -167,6 +167,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_net_finalize(net, tn->trial_addr);
> +   msg_set_prevnode(buf_msg(d->skb), tn->trial_addr);
> msg_set_type(buf_msg(d->skb), DSC_REQ_MSG);
> }
> 
> @@ -300,14 +301,12 @@ static void tipc_disc_timeout(struct timer_list *t)
> goto exit;
> }
> 
> -   /* Trial period over ? */
> -   if (!time_before(jiffies, tn->addr_trial_end)) {
> -   /* Did we just leave it ? */
> -   if (!tipc_own_addr(net))
> -   tipc_net_finalize(net, tn->trial_addr);
> -
> -   msg_set_type(buf_msg(d->skb), DSC_REQ_MSG);
> -   msg_set_prevnode(buf_msg(d->skb), tipc_own_addr(net));
> +   /* Did we just leave trial period ? */
> +   if (!time_before(jiffies, tn->addr_trial_end) && !tipc_own_addr(net)) 
> {
> +   mod_timer(>timer, jiffies + TIPC_DISC_INIT);
> +   spin_unlock_bh(>lock);
> +   tipc_net_finalize(net, tn->trial_addr);
> +   return;
> }
> 
> /* Adjust timeout interval according to discovery phase */
> @@ -319,6 +318,8 @@ static void tipc_disc_timeout(struct timer_list *t)
> d->timer_intv = TIPC_DISC_SLOW;
> else if (!d->num_nodes && d->timer_intv > TIPC_DISC_FAST)
> d->timer_intv = TIPC_DISC_FAST;
> +   msg_set_type(buf_msg(d->skb), DSC_REQ_MSG);
> +   msg_set_prevnode(buf_msg(d->skb), tn->trial_addr);
> }
> 
> mod_timer(>timer, jiffies + d->timer_intv);
> --
> 2.1.4
> 
> 
> 
> 
>> -Original Message-
>> From: Ying Xue 
>> Sent: 5-Nov-18 06:34
>> To: Jon Maloy 
>> Subject: Re: Deadlock warning
>>
>> FYI: I have found its root cause. Now I am figuring out how to a bit 
>> elegantly
>> solve it.
>>
>> On 10/17/2018 10:36 AM, Jon Maloy wrote:
>>> Hi Ying,
>>>
>>> I sometimes get the following deadlock warning. As I understand it the
>>> reason is that we are calling rhashtable_walk_enter() in a tmer, i.e.,
>>> in an SW interrupt, something that is not permitted. Do you agree with
>>> this interpretation? Since you have worked more with these hash tables

Re: [tipc-discussion] TIPC scalability viewpoints

[Ying Xue]

Hi Juhamatti,

This is a good test plan indeed. If you encounter any problem during the
testing, please share it here.

>From my view, TIPC had fully supported networking nemespace at least
three years ago and its sockets could be fully isolated between containers.

Thanks,
Ying

On 10/26/2018 11:12 AM, juham...@gmail.com wrote:
> Hello,
> 
> I'm planning to test TIPC scalability in more detail, especially
> regarding the binding table and topology (service tracking) service
> with large number of nodes (~1k) and sockets (100-1k). Is it enough to
> do it with LXC containers with isolated namespaces to get realistic
> results? My concern here is that if TIPC implementation would not
> provide full isolation to sockets e.g. for performance reasons inside
> the same kernel, then this may not provide results matching to real
> environments. VMs are the other choice, but they tend to be heavy. Are
> the namespaces fully isolated inside the kernel?
> 
> Thanks for the info,
> --
>  Juhamatti
> 
> 
> ___
> tipc-discussion mailing list
> tipc-discussion@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH] tipc: use destination length for copy string

[Ying Xue]

On 10/19/2018 12:08 PM, Guoqing Jiang wrote:
> Got below warning with gcc 8.2 compiler.
> 
> net/tipc/topsrv.c: In function ‘tipc_topsrv_start’:
> net/tipc/topsrv.c:660:2: warning: ‘strncpy’ specified bound depends on the 
> length of the source argument [-Wstringop-overflow=]
>   strncpy(srv->name, name, strlen(name) + 1);
>   ^~
> net/tipc/topsrv.c:660:27: note: length computed here
>   strncpy(srv->name, name, strlen(name) + 1);
>^~~~
> So change it to correct length and use strscpy.
> 
> Signed-off-by: Guoqing Jiang 

Acked-by: Ying Xue 

> ---
>  net/tipc/topsrv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index 2627b5d812e9..b84c0059214f 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c
> @@ -657,7 +657,7 @@ int tipc_topsrv_start(struct net *net)
>   srv->max_rcvbuf_size = sizeof(struct tipc_subscr);
>   INIT_WORK(>awork, tipc_topsrv_accept);
>  
> - strncpy(srv->name, name, strlen(name) + 1);
> + strscpy(srv->name, name, sizeof(srv->name));
>   tn->topsrv = srv;
>   atomic_set(>subscription_count, 0);
>  
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net 1/1] tipc: initialize broadcast link stale counter correctly

[Ying Xue]

On 10/12/2018 04:02 AM, Jon Maloy wrote:
> In the commit referred to below we added link tolerance as an additional
> criteria for declaring broadcast transmission "stale" and resetting the
> unicast links to the affected node.
> 
> Unfortunately, this 'improvement' introduced two bugs, which each and
> one alone cause only limited problems, but combined lead to seemingly
> stochastic unicast link resets, depending on the amount of broadcast
> traffic transmitted.
> 
> The first issue, a missing initialization of the 'tolerance' field of
> the receiver broadcast link, was recently fixed by commit 047491ea334a
> ("tipc: set link tolerance correctly in broadcast link").
> 
> Ths second issue, where we omit to reset the 'stale_cnt' field of
> the same link after a 'stale' period is over, leads to this counter
> accumulating over time, and in the absence of the 'tolerance' criteria
> leads to the above described symptoms. This commit adds the missing
> initialization.
> 
> Fixes: a4dc70d46cf1 ("tipc: extend link reset criteria for stale packet
> retransmission")
> 
> Signed-off-by: Jon Maloy 

Acked-by: Ying Xue 

> ---
>  net/tipc/link.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/tipc/link.c b/net/tipc/link.c
> index f6552e4..201c3b5 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -1041,6 +1041,7 @@ static int tipc_link_retrans(struct tipc_link *l, 
> struct tipc_link *r,
>   if (r->last_retransm != buf_seqno(skb)) {
>   r->last_retransm = buf_seqno(skb);
>   r->stale_limit = jiffies + msecs_to_jiffies(r->tolerance);
> + r->stale_cnt = 0;
>   } else if (++r->stale_cnt > 99 && time_after(jiffies, r->stale_limit)) {
>   link_retransmit_failure(l, skb);
>   if (link_is_bc_sndlink(l))
> 


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] net/tipc: recursive locking in tipc_link_reset

[Ying Xue]

Jon, please help to review the patch:
https://patchwork.ozlabs.org/patch/982447.

Thanks,
Ying

On 10/11/2018 06:55 PM, Jon Maloy wrote:
> Hi Dmitry,
> Yes, we are aware of this, the kernel test robot warned us about this a few 
> days ago.
> I am looking into it.
> 
> ///jon


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] net/tipc: recursive locking in tipc_link_reset

[Ying Xue]

On 10/11/2018 03:59 PM, Dmitry Vyukov wrote:
> On Thu, Oct 11, 2018 at 9:55 AM, Dmitry Vyukov  wrote:
>> Hi,
>>
>> I am getting the following error while booting the latest kernel on
>> bb2d8f2f61047cbde08b78ec03e4ebdb01ee5434 (Oct 10). Config is attached.
>>
>> Since this happens during boot, this makes LOCKDEP completely
>> unusable, does not allow to discover any other locking issues and
>> masks all new bugs being introduced into kernel.
>> Please fix asap.
>> Thanks
> -parthasarathy.bhuvaragan address as it gives me bounces
> but this is highly likely due to:
> 
> commit 3f32d0be6c16b902b687453c962d17eea5b8ea19
> Author: Parthasarathy Bhuvaragan
> Date:   Tue Sep 25 22:09:10 2018 +0200
> 
> tipc: lock wakeup & inputq at tipc_link_reset()
> 
> 

Dmitry, I agree with you. The complaint should be caused by the commit
above. Please try to verify the patch:
https://patchwork.ozlabs.org/patch/982447.

Thanks,
Ying


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [PATCH net] tipc: eliminate possible recursive locking detected by LOCKDEP

[Ying Xue]

When booting kernel with LOCKDEP option, below warning info was found:

WARNING: possible recursive locking detected
4.19.0-rc7+ #14 Not tainted

swapper/0/1 is trying to acquire lock:
dcfc0fc8 (&(>lock)->rlock#4){+...}, at: spin_lock_bh
include/linux/spinlock.h:334 [inline]
dcfc0fc8 (&(>lock)->rlock#4){+...}, at:
tipc_link_reset+0x125/0xdf0 net/tipc/link.c:850

but task is already holding lock:
cbb9b036 (&(>lock)->rlock#4){+...}, at: spin_lock_bh
include/linux/spinlock.h:334 [inline]
cbb9b036 (&(>lock)->rlock#4){+...}, at:
tipc_link_reset+0xfa/0xdf0 net/tipc/link.c:849

other info that might help us debug this:
 Possible unsafe locking scenario:

   CPU0
   
  lock(&(>lock)->rlock#4);
  lock(&(>lock)->rlock#4);

 *** DEADLOCK ***

 May be due to missing lock nesting notation

2 locks held by swapper/0/1:
 #0: f7539d34 (pernet_ops_rwsem){+.+.}, at:
register_pernet_subsys+0x19/0x40 net/core/net_namespace.c:1051
 #1: cbb9b036 (&(>lock)->rlock#4){+...}, at:
spin_lock_bh include/linux/spinlock.h:334 [inline]
 #1: cbb9b036 (&(>lock)->rlock#4){+...}, at:
tipc_link_reset+0xfa/0xdf0 net/tipc/link.c:849

stack backtrace:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.19.0-rc7+ #14
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x1af/0x295 lib/dump_stack.c:113
 print_deadlock_bug kernel/locking/lockdep.c:1759 [inline]
 check_deadlock kernel/locking/lockdep.c:1803 [inline]
 validate_chain kernel/locking/lockdep.c:2399 [inline]
 __lock_acquire+0xf1e/0x3c60 kernel/locking/lockdep.c:3411
 lock_acquire+0x1db/0x520 kernel/locking/lockdep.c:3900
 __raw_spin_lock_bh include/linux/spinlock_api_smp.h:135 [inline]
 _raw_spin_lock_bh+0x31/0x40 kernel/locking/spinlock.c:168
 spin_lock_bh include/linux/spinlock.h:334 [inline]
 tipc_link_reset+0x125/0xdf0 net/tipc/link.c:850
 tipc_link_bc_create+0xb5/0x1f0 net/tipc/link.c:526
 tipc_bcast_init+0x59b/0xab0 net/tipc/bcast.c:521
 tipc_init_net+0x472/0x610 net/tipc/core.c:82
 ops_init+0xf7/0x520 net/core/net_namespace.c:129
 __register_pernet_operations net/core/net_namespace.c:940 [inline]
 register_pernet_operations+0x453/0xac0 net/core/net_namespace.c:1011
 register_pernet_subsys+0x28/0x40 net/core/net_namespace.c:1052
 tipc_init+0x83/0x104 net/tipc/core.c:140
 do_one_initcall+0x109/0x70a init/main.c:885
 do_initcall_level init/main.c:953 [inline]
 do_initcalls init/main.c:961 [inline]
 do_basic_setup init/main.c:979 [inline]
 kernel_init_freeable+0x4bd/0x57f init/main.c:1144
 kernel_init+0x13/0x180 init/main.c:1063
 ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:413

The reason why the noise above was complained by LOCKDEP is because we
nested to hold l->wakeupq.lock and l->inputq->lock in tipc_link_reset
function. In fact it's unnecessary to move skb buffer from l->wakeupq
queue to l->inputq queue while holding the two locks at the same time.
Instead, we can move skb buffers in l->wakeupq queue to a temporary
list first and then move the buffers of the temporary list to l->inputq
queue, which is also safe for us.

Fixes: 3f32d0be6c16 ("tipc: lock wakeup & inputq at tipc_link_reset()")
Reported-by: Dmitry Vyukov 
Signed-off-by: Ying Xue 
---
 net/tipc/link.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index fb886b5..1d21ae4 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -843,14 +843,21 @@ static void link_prepare_wakeup(struct tipc_link *l)
 
 void tipc_link_reset(struct tipc_link *l)
 {
+   struct sk_buff_head list;
+
+   __skb_queue_head_init();
+
l->in_session = false;
l->session++;
l->mtu = l->advertised_mtu;
+
spin_lock_bh(>wakeupq.lock);
+   skb_queue_splice_init(>wakeupq, );
+   spin_unlock_bh(>wakeupq.lock);
+
spin_lock_bh(>inputq->lock);
-   skb_queue_splice_init(>wakeupq, l->inputq);
+   skb_queue_splice_init(, l->inputq);
spin_unlock_bh(>inputq->lock);
-   spin_unlock_bh(>wakeupq.lock);
 
__skb_queue_purge(>transmq);
__skb_queue_purge(>deferdq);
-- 
2.7.4



___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion