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

[tipc-discussion] [net-next 1/1] tipc: simplify stale link failure criteria

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

In commit a4dc70d46cf1 ("tipc: extend link reset criteria for stale
packet retransmission") we made link retransmission failure events
dependent on the link tolerance, and not only of the number of failed
retransmission attempts, as we did earlier. This works well. However,
keeping the original, additional criteria of 99 failed retransmissions
is now redundant, and may in some cases lead to failure detection
times in the order of minutes instead of the expected 1.5 sec link
tolerance value.

We now remove this criteria altogether.

Acked-by: Ying Xue <yin...@wi...>
Signed-off-by: Jon Maloy <jon...@er...>
---
 net/tipc/link.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index bcfb0a4..af50b53 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -107,7 +107,6 @@ struct tipc_stats {
  * @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_cnt: counter for number of identical retransmit attempts
  * @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.
@@ -167,7 +166,6 @@ struct tipc_link {
 	u16 snd_nxt;
 	u16 prev_from;
 	u16 window;
-	u16 stale_cnt;
 	unsigned long stale_limit;
 
 	/* Reception */
@@ -910,7 +908,6 @@ void tipc_link_reset(struct tipc_link *l)
 	l->acked = 0;
 	l->silent_intv_cnt = 0;
 	l->rst_cnt = 0;
-	l->stale_cnt = 0;
 	l->bc_peer_is_up = false;
 	memset(&l->mon_state, 0, sizeof(l->mon_state));
 	tipc_link_reset_stats(l);
@@ -1068,8 +1065,7 @@ static bool link_retransmit_failure(struct tipc_link *l, struct tipc_link *r,
 	if (r->prev_from != from) {
 		r->prev_from = from;
 		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)) {
+	} 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",
@@ -1515,7 +1511,6 @@ int tipc_link_rcv(struct tipc_link *l, struct sk_buff *skb,
 
 		/* Forward queues and wake up waiting users */
 		if (likely(tipc_link_release_pkts(l, msg_ack(hdr)))) {
-			l->stale_cnt = 0;
 			tipc_link_advance_backlog(l, xmitq);
 			if (unlikely(!skb_queue_empty(&l->wakeupq)))
 				link_prepare_wakeup(l);
@@ -2584,7 +2579,7 @@ int tipc_link_dump(struct tipc_link *l, u16 dqueues, char *buf)
 	i += scnprintf(buf + i, sz - i, " %u", l->silent_intv_cnt);
 	i += scnprintf(buf + i, sz - i, " %u", l->rst_cnt);
 	i += scnprintf(buf + i, sz - i, " %u", l->prev_from);
-	i += scnprintf(buf + i, sz - i, " %u", l->stale_cnt);
+	i += scnprintf(buf + i, sz - i, " %u", 0);
 	i += scnprintf(buf + i, sz - i, " %u", l->acked);
 
 	list = &l->transmq;
-- 
2.1.4

[tipc-discussion] [iproute2-next v6] tipc: support interface name when activating UDP bearer

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

Support for indicating interface name has an ip address in parallel
with specifying ip address when activating UDP bearer.
This liberates the user from keeping track of the current ip address
for each device.

Old command syntax:
$tipc bearer enable media udp name NAME localip IP

New command syntax:
$tipc bearer enable media udp name NAME [localip IP|dev DEVICE]

v2:
    - Removed initial value for fd
    - Fixed the returning value for cmd_bearer_validate_and_get_addr
      to make its consistent with using: zero or non-zero
v3: - Switch to use helper 'get_ifname' to retrieve interface name
v4: - Replace legacy SIOCGIFADDR by netlink
v5: - Fix leaky rtnl_handle

Acked-by: Ying Xue <yin...@wi...>
Signed-off-by: Hoang Le <hoa...@de...>
---
 tipc/bearer.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 89 insertions(+), 5 deletions(-)

diff --git a/tipc/bearer.c b/tipc/bearer.c
index 1f3a4d44441e..4470819e4a96 100644
--- a/tipc/bearer.c
+++ b/tipc/bearer.c
@@ -19,10 +19,12 @@
 #include <linux/tipc_netlink.h>
 #include <linux/tipc.h>
 #include <linux/genetlink.h>
+#include <linux/if.h>
 
 #include <libmnl/libmnl.h>
 #include <sys/socket.h>
 
+#include "utils.h"
 #include "cmdl.h"
 #include "msg.h"
 #include "bearer.h"
@@ -68,7 +70,7 @@ static void cmd_bearer_enable_l2_help(struct cmdl *cmdl, char *media)
 static void cmd_bearer_enable_udp_help(struct cmdl *cmdl, char *media)
 {
 	fprintf(stderr,
-		"Usage: %s bearer enable [OPTIONS] media %s name NAME localip IP [UDP OPTIONS]\n\n"
+		"Usage: %s bearer enable [OPTIONS] media %s name NAME [localip IP|device DEVICE] [UDP OPTIONS]\n\n"
 		"OPTIONS\n"
 		" domain DOMAIN		- Discovery domain\n"
 		" priority PRIORITY	- Bearer priority\n\n"
@@ -119,6 +121,76 @@ static int generate_multicast(short af, char *buf, int bufsize)
 	return 0;
 }
 
+static struct ifreq ifr;
+static int nl_dump_req_filter(struct nlmsghdr *nlh, int reqlen)
+{
+	struct ifaddrmsg *ifa = NLMSG_DATA(nlh);
+
+	ifa->ifa_index = ifr.ifr_ifindex;
+
+	return 0;
+}
+
+static int nl_dump_addr_filter(struct nlmsghdr *nlh, void *arg)
+{
+	struct ifaddrmsg *ifa = NLMSG_DATA(nlh);
+	char *r_addr = (char *)arg;
+	int len = nlh->nlmsg_len;
+	struct rtattr *addr_attr;
+
+	if (ifr.ifr_ifindex != ifa->ifa_index)
+		return 0;
+
+	if (strlen(r_addr) > 0)
+		return 0;
+
+	addr_attr = parse_rtattr_one(IFA_ADDRESS, IFA_RTA(ifa),
+				     len - NLMSG_LENGTH(sizeof(*ifa)));
+	if (!addr_attr)
+		return 0;
+
+	if (ifa->ifa_family == AF_INET) {
+		struct sockaddr_in ip4addr;
+		memcpy(&ip4addr.sin_addr, RTA_DATA(addr_attr),
+		       sizeof(struct in_addr));
+		inet_ntop(AF_INET, &ip4addr.sin_addr, r_addr,
+			  INET_ADDRSTRLEN);
+	} else if (ifa->ifa_family == AF_INET6) {
+		struct sockaddr_in6 ip6addr;
+		memcpy(&ip6addr.sin6_addr, RTA_DATA(addr_attr),
+		       sizeof(struct in6_addr));
+		inet_ntop(AF_INET6, &ip6addr.sin6_addr, r_addr,
+			  INET6_ADDRSTRLEN);
+	}
+	return 0;
+}
+
+static int cmd_bearer_validate_and_get_addr(const char *name, char *r_addr)
+{
+	struct rtnl_handle rth = { .fd = -1 };
+	int err = -1;
+
+	memset(&ifr, 0, sizeof(ifr));
+	if (!name || !r_addr || get_ifname(ifr.ifr_name, name))
+		return err;
+
+	ifr.ifr_ifindex = ll_name_to_index(ifr.ifr_name);
+	if (!ifr.ifr_ifindex)
+		return err;
+
+	/* remove from cache */
+	ll_drop_by_index(ifr.ifr_ifindex);
+
+	if ((err = rtnl_open(&rth, 0)) < 0)
+		return err;
+
+	if ((err = rtnl_addrdump_req(&rth, AF_UNSPEC, nl_dump_req_filter)) > 0)
+		err = rtnl_dump_filter(&rth, nl_dump_addr_filter, r_addr);
+
+	rtnl_close(&rth);
+	return err;
+}
+
 static int nl_add_udp_enable_opts(struct nlmsghdr *nlh, struct opt *opts,
 				  struct cmdl *cmdl)
 {
@@ -136,13 +208,25 @@ static int nl_add_udp_enable_opts(struct nlmsghdr *nlh, struct opt *opts,
 		.ai_family = AF_UNSPEC,
 		.ai_socktype = SOCK_DGRAM
 	};
+	char addr[INET6_ADDRSTRLEN] = {0};
 
-	if (!(opt = get_opt(opts, "localip"))) {
-		fprintf(stderr, "error, udp bearer localip missing\n");
-		cmd_bearer_enable_udp_help(cmdl, "udp");
+	opt = get_opt(opts, "device");
+	if (opt && cmd_bearer_validate_and_get_addr(opt->val, addr) < 0) {
+		fprintf(stderr, "error, no device name available\n");
 		return -EINVAL;
 	}
-	locip = opt->val;
+
+	if (strlen(addr) > 0) {
+		locip = addr;
+	} else {
+		opt = get_opt(opts, "localip");
+		if (!opt) {
+			fprintf(stderr, "error, udp bearer localip/device missing\n");
+			cmd_bearer_enable_udp_help(cmdl, "udp");
+			return -EINVAL;
+		}
+		locip = opt->val;
+	}
 
 	if ((opt = get_opt(opts, "remoteip")))
 		remip = opt->val;
-- 
2.17.1

Re: [tipc-discussion] [PATCHv2 net] tipc: check msg->req data len in tipc_nl_compat_bearer_disable

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

From: Xin Long <luc...@gm...>
Date: Tue, 25 Jun 2019 00:28:19 +0800

> This patch is to fix an uninit-value issue, reported by syzbot:
 ...
> TLV_GET_DATA_LEN() may return a negtive int value, which will be
> used as size_t (becoming a big unsigned long) passed into memchr,
> cause this issue.
> 
> Similar to what it does in tipc_nl_compat_bearer_enable(), this
> fix is to return -EINVAL when TLV_GET_DATA_LEN() is negtive in
> tipc_nl_compat_bearer_disable(), as well as in
> tipc_nl_compat_link_stat_dump() and tipc_nl_compat_link_reset_stats().
> 
> v1->v2:
>   - add the missing Fixes tags per Eric's request.
> 
> Fixes: 0762216c0ad2 ("tipc: fix uninit-value in tipc_nl_compat_bearer_enable")
> Fixes: 8b66fee7f8ee ("tipc: fix uninit-value in tipc_nl_compat_link_reset_stats")
> Reported-by: syz...@sy...
> Signed-off-by: Xin Long <luc...@gm...>

Applied and queued up for -stable, thanks.

[tipc-discussion] [PATCHv2 net] tipc: check msg->req data len in tipc_nl_compat_bearer_disable

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

This patch is to fix an uninit-value issue, reported by syzbot:

  BUG: KMSAN: uninit-value in memchr+0xce/0x110 lib/string.c:981
  Call Trace:
    __dump_stack lib/dump_stack.c:77 [inline]
    dump_stack+0x191/0x1f0 lib/dump_stack.c:113
    kmsan_report+0x130/0x2a0 mm/kmsan/kmsan.c:622
    __msan_warning+0x75/0xe0 mm/kmsan/kmsan_instr.c:310
    memchr+0xce/0x110 lib/string.c:981
    string_is_valid net/tipc/netlink_compat.c:176 [inline]
    tipc_nl_compat_bearer_disable+0x2a1/0x480 net/tipc/netlink_compat.c:449
    __tipc_nl_compat_doit net/tipc/netlink_compat.c:327 [inline]
    tipc_nl_compat_doit+0x3ac/0xb00 net/tipc/netlink_compat.c:360
    tipc_nl_compat_handle net/tipc/netlink_compat.c:1178 [inline]
    tipc_nl_compat_recv+0x1b1b/0x27b0 net/tipc/netlink_compat.c:1281

TLV_GET_DATA_LEN() may return a negtive int value, which will be
used as size_t (becoming a big unsigned long) passed into memchr,
cause this issue.

Similar to what it does in tipc_nl_compat_bearer_enable(), this
fix is to return -EINVAL when TLV_GET_DATA_LEN() is negtive in
tipc_nl_compat_bearer_disable(), as well as in
tipc_nl_compat_link_stat_dump() and tipc_nl_compat_link_reset_stats().

v1->v2:
  - add the missing Fixes tags per Eric's request.

Fixes: 0762216c0ad2 ("tipc: fix uninit-value in tipc_nl_compat_bearer_enable")
Fixes: 8b66fee7f8ee ("tipc: fix uninit-value in tipc_nl_compat_link_reset_stats")
Reported-by: syz...@sy...
Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/netlink_compat.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index c6a04c0..cf15506 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -445,7 +445,11 @@ static int tipc_nl_compat_bearer_disable(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);
+	len = TLV_GET_DATA_LEN(msg->req);
+	if (len <= 0)
+		return -EINVAL;
+
+	len = min_t(int, len, TIPC_MAX_BEARER_NAME);
 	if (!string_is_valid(name, len))
 		return -EINVAL;
 
@@ -539,7 +543,11 @@ static int tipc_nl_compat_link_stat_dump(struct tipc_nl_compat_msg *msg,
 
 	name = (char *)TLV_DATA(msg->req);
 
-	len = min_t(int, TLV_GET_DATA_LEN(msg->req), TIPC_MAX_LINK_NAME);
+	len = TLV_GET_DATA_LEN(msg->req);
+	if (len <= 0)
+		return -EINVAL;
+
+	len = min_t(int, len, TIPC_MAX_BEARER_NAME);
 	if (!string_is_valid(name, len))
 		return -EINVAL;
 
@@ -817,7 +825,11 @@ static int tipc_nl_compat_link_reset_stats(struct tipc_nl_compat_cmd_doit *cmd,
 	if (!link)
 		return -EMSGSIZE;
 
-	len = min_t(int, TLV_GET_DATA_LEN(msg->req), TIPC_MAX_LINK_NAME);
+	len = TLV_GET_DATA_LEN(msg->req);
+	if (len <= 0)
+		return -EINVAL;
+
+	len = min_t(int, len, TIPC_MAX_BEARER_NAME);
 	if (!string_is_valid(name, len))
 		return -EINVAL;
 
-- 
2.1.0

Re: [tipc-discussion] [PATCH net] tipc: check msg->req data len in tipc_nl_compat_bearer_disable

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

From: Xin Long <luc...@gm...>
Date: Tue, 25 Jun 2019 00:00:39 +0800

> Sorry, David, do I need to resend this one?

Yes, please, that helps me a lot.

Re: [tipc-discussion] [PATCH net] tipc: check msg->req data len in tipc_nl_compat_bearer_disable

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

On Mon, Jun 24, 2019 at 4:33 PM Eric Dumazet <eri...@gm...> wrote:
>
>
>
> On 6/24/19 12:59 AM, Xin Long wrote:
> > This patch is to fix an uninit-value issue, reported by syzbot:
> >
> >   BUG: KMSAN: uninit-value in memchr+0xce/0x110 lib/string.c:981
> >   Call Trace:
> >     __dump_stack lib/dump_stack.c:77 [inline]
> >     dump_stack+0x191/0x1f0 lib/dump_stack.c:113
> >     kmsan_report+0x130/0x2a0 mm/kmsan/kmsan.c:622
> >     __msan_warning+0x75/0xe0 mm/kmsan/kmsan_instr.c:310
> >     memchr+0xce/0x110 lib/string.c:981
> >     string_is_valid net/tipc/netlink_compat.c:176 [inline]
> >     tipc_nl_compat_bearer_disable+0x2a1/0x480 net/tipc/netlink_compat.c:449
> >     __tipc_nl_compat_doit net/tipc/netlink_compat.c:327 [inline]
> >     tipc_nl_compat_doit+0x3ac/0xb00 net/tipc/netlink_compat.c:360
> >     tipc_nl_compat_handle net/tipc/netlink_compat.c:1178 [inline]
> >     tipc_nl_compat_recv+0x1b1b/0x27b0 net/tipc/netlink_compat.c:1281
> >
> > TLV_GET_DATA_LEN() may return a negtive int value, which will be
> > used as size_t (becoming a big unsigned long) passed into memchr,
> > cause this issue.
> >
> > Similar to what it does in tipc_nl_compat_bearer_enable(), this
> > fix is to return -EINVAL when TLV_GET_DATA_LEN() is negtive in
> > tipc_nl_compat_bearer_disable(), as well as in
> > tipc_nl_compat_link_stat_dump() and tipc_nl_compat_link_reset_stats().
> >
> > Reported-by: syz...@sy...
> > Signed-off-by: Xin Long <luc...@gm...>
>
> Please add an appropriate Fixes: tag, thanks !
>
Fixes: 0762216c0ad2 ("tipc: fix uninit-value in tipc_nl_compat_bearer_enable")
Fixes: 8b66fee7f8ee (:tipc: fix uninit-value in
tipc_nl_compat_link_reset_stats")

Sorry, David, do I need to resend this one?

Re: [tipc-discussion] [PATCH net] tipc: remove the unnecessary msg->req check from tipc_nl_compat_bearer_set

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

From: Xin Long <luc...@gm...>
Date: Mon, 24 Jun 2019 23:54:02 +0800

> On Mon, Jun 24, 2019 at 10:35 PM David Miller <da...@da...> wrote:
>>
>> From: Xin Long <luc...@gm...>
>> Date: Mon, 24 Jun 2019 16:02:42 +0800
>>
>> > tipc_nl_compat_bearer_set() is only called by tipc_nl_compat_link_set()
>> > which already does the check for msg->req check, so remove it from
>> > tipc_nl_compat_bearer_set(), and do the same in tipc_nl_compat_media_set().
>> >
>> > Signed-off-by: Xin Long <luc...@gm...>
>>
>> Is this really appropriate as a fix for 'net'?  Seems more like net-next material
>> to me.
> kind of code fix, sure, you can apply it to net-next, no conflict.
> do you need me to repost?

No need to resend, applied to net-next, thanks!

Re: [tipc-discussion] [PATCH net] tipc: remove the unnecessary msg->req check from tipc_nl_compat_bearer_set

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

On Mon, Jun 24, 2019 at 10:35 PM David Miller <da...@da...> wrote:
>
> From: Xin Long <luc...@gm...>
> Date: Mon, 24 Jun 2019 16:02:42 +0800
>
> > tipc_nl_compat_bearer_set() is only called by tipc_nl_compat_link_set()
> > which already does the check for msg->req check, so remove it from
> > tipc_nl_compat_bearer_set(), and do the same in tipc_nl_compat_media_set().
> >
> > Signed-off-by: Xin Long <luc...@gm...>
>
> Is this really appropriate as a fix for 'net'?  Seems more like net-next material
> to me.
kind of code fix, sure, you can apply it to net-next, no conflict.
do you need me to repost?

Re: [tipc-discussion] [PATCH net] tipc: remove the unnecessary msg->req check from tipc_nl_compat_bearer_set

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

From: Xin Long <luc...@gm...>
Date: Mon, 24 Jun 2019 16:02:42 +0800

> tipc_nl_compat_bearer_set() is only called by tipc_nl_compat_link_set()
> which already does the check for msg->req check, so remove it from
> tipc_nl_compat_bearer_set(), and do the same in tipc_nl_compat_media_set().
> 
> Signed-off-by: Xin Long <luc...@gm...>

Is this really appropriate as a fix for 'net'?  Seems more like net-next material
to me.

Re: [tipc-discussion] [net-next] tipc: fix missing indentation in source code

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

From: John Rutherford <joh...@de...>
Date: Mon, 24 Jun 2019 14:01:23 +1000

> Fix misalignment of policy statement in netlink.c due to automatic
> spatch code transformation.
> 
> Fixes: 3b0f31f2b8c9 ("genetlink: make policy common to family")
> Acked-by: Jon Maloy <jon...@er...>
> Signed-off-by: John Rutherford <joh...@de...>

Applied.

[tipc-discussion] [PATCH net] tipc: remove the unnecessary msg->req check from tipc_nl_compat_bearer_set

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

tipc_nl_compat_bearer_set() is only called by tipc_nl_compat_link_set()
which already does the check for msg->req check, so remove it from
tipc_nl_compat_bearer_set(), and do the same in tipc_nl_compat_media_set().

Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/netlink_compat.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index cf15506..d86030e 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -691,7 +691,6 @@ static int tipc_nl_compat_media_set(struct sk_buff *skb,
 	struct nlattr *prop;
 	struct nlattr *media;
 	struct tipc_link_config *lc;
-	int len;
 
 	lc = (struct tipc_link_config *)TLV_DATA(msg->req);
 
@@ -699,10 +698,6 @@ static int tipc_nl_compat_media_set(struct sk_buff *skb,
 	if (!media)
 		return -EMSGSIZE;
 
-	len = min_t(int, TLV_GET_DATA_LEN(msg->req), TIPC_MAX_MEDIA_NAME);
-	if (!string_is_valid(lc->name, len))
-		return -EINVAL;
-
 	if (nla_put_string(skb, TIPC_NLA_MEDIA_NAME, lc->name))
 		return -EMSGSIZE;
 
@@ -723,7 +718,6 @@ static int tipc_nl_compat_bearer_set(struct sk_buff *skb,
 	struct nlattr *prop;
 	struct nlattr *bearer;
 	struct tipc_link_config *lc;
-	int len;
 
 	lc = (struct tipc_link_config *)TLV_DATA(msg->req);
 
@@ -731,10 +725,6 @@ static int tipc_nl_compat_bearer_set(struct sk_buff *skb,
 	if (!bearer)
 		return -EMSGSIZE;
 
-	len = min_t(int, TLV_GET_DATA_LEN(msg->req), TIPC_MAX_MEDIA_NAME);
-	if (!string_is_valid(lc->name, len))
-		return -EINVAL;
-
 	if (nla_put_string(skb, TIPC_NLA_BEARER_NAME, lc->name))
 		return -EMSGSIZE;
 
-- 
2.1.0

[tipc-discussion] [PATCH net] tipc: check msg->req data len in tipc_nl_compat_bearer_disable

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

This patch is to fix an uninit-value issue, reported by syzbot:

  BUG: KMSAN: uninit-value in memchr+0xce/0x110 lib/string.c:981
  Call Trace:
    __dump_stack lib/dump_stack.c:77 [inline]
    dump_stack+0x191/0x1f0 lib/dump_stack.c:113
    kmsan_report+0x130/0x2a0 mm/kmsan/kmsan.c:622
    __msan_warning+0x75/0xe0 mm/kmsan/kmsan_instr.c:310
    memchr+0xce/0x110 lib/string.c:981
    string_is_valid net/tipc/netlink_compat.c:176 [inline]
    tipc_nl_compat_bearer_disable+0x2a1/0x480 net/tipc/netlink_compat.c:449
    __tipc_nl_compat_doit net/tipc/netlink_compat.c:327 [inline]
    tipc_nl_compat_doit+0x3ac/0xb00 net/tipc/netlink_compat.c:360
    tipc_nl_compat_handle net/tipc/netlink_compat.c:1178 [inline]
    tipc_nl_compat_recv+0x1b1b/0x27b0 net/tipc/netlink_compat.c:1281

TLV_GET_DATA_LEN() may return a negtive int value, which will be
used as size_t (becoming a big unsigned long) passed into memchr,
cause this issue.

Similar to what it does in tipc_nl_compat_bearer_enable(), this
fix is to return -EINVAL when TLV_GET_DATA_LEN() is negtive in
tipc_nl_compat_bearer_disable(), as well as in
tipc_nl_compat_link_stat_dump() and tipc_nl_compat_link_reset_stats().

Reported-by: syz...@sy...
Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/netlink_compat.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index c6a04c0..cf15506 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -445,7 +445,11 @@ static int tipc_nl_compat_bearer_disable(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);
+	len = TLV_GET_DATA_LEN(msg->req);
+	if (len <= 0)
+		return -EINVAL;
+
+	len = min_t(int, len, TIPC_MAX_BEARER_NAME);
 	if (!string_is_valid(name, len))
 		return -EINVAL;
 
@@ -539,7 +543,11 @@ static int tipc_nl_compat_link_stat_dump(struct tipc_nl_compat_msg *msg,
 
 	name = (char *)TLV_DATA(msg->req);
 
-	len = min_t(int, TLV_GET_DATA_LEN(msg->req), TIPC_MAX_LINK_NAME);
+	len = TLV_GET_DATA_LEN(msg->req);
+	if (len <= 0)
+		return -EINVAL;
+
+	len = min_t(int, len, TIPC_MAX_BEARER_NAME);
 	if (!string_is_valid(name, len))
 		return -EINVAL;
 
@@ -817,7 +825,11 @@ static int tipc_nl_compat_link_reset_stats(struct tipc_nl_compat_cmd_doit *cmd,
 	if (!link)
 		return -EMSGSIZE;
 
-	len = min_t(int, TLV_GET_DATA_LEN(msg->req), TIPC_MAX_LINK_NAME);
+	len = TLV_GET_DATA_LEN(msg->req);
+	if (len <= 0)
+		return -EINVAL;
+
+	len = min_t(int, len, TIPC_MAX_BEARER_NAME);
 	if (!string_is_valid(name, len))
 		return -EINVAL;
 
-- 
2.1.0

[tipc-discussion] [net-next] tipc: fix missing indentation in source code

[John R. <joh...@de...>]

Fix misalignment of policy statement in netlink.c due to automatic
spatch code transformation.

Fixes: 3b0f31f2b8c9 ("genetlink: make policy common to family")
Acked-by: Jon Maloy <jon...@er...>
Signed-off-by: John Rutherford <joh...@de...>
---
 net/tipc/netlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tipc/netlink.c b/net/tipc/netlink.c
index 99bd166..d6165ad 100644
--- a/net/tipc/netlink.c
+++ b/net/tipc/netlink.c
@@ -261,7 +261,7 @@ struct genl_family tipc_genl_family __ro_after_init = {
 	.version	= TIPC_GENL_V2_VERSION,
 	.hdrsize	= 0,
 	.maxattr	= TIPC_NLA_MAX,
-	.policy = tipc_nl_policy,
+	.policy		= tipc_nl_policy,
 	.netnsok	= true,
 	.module		= THIS_MODULE,
 	.ops		= tipc_genl_v2_ops,
-- 
2.11.0

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

[John R. <joh...@de...>]

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".

Acked-by: Jon Maloy <jon...@er...>
Signed-off-by: John Rutherford <joh...@de...>
---
 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, &localq);
 		tipc_sk_mcast_rcv(net, &localq, &inputq);
+	}
 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(&tn->loopback_pt);
+	return 0;
+}
+
+void tipc_detach_loopback(struct net *net)
+{
+	struct tipc_net *tn = tipc_net(net);
+
+	dev_remove_pack(&tn->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,
 		      struct tipc_media_addr *dst);
 void tipc_bearer_bc_xmit(struct net *net, u32 bearer_id,
 			 struct sk_buff_head *xmitq);
+void tipc_clone_to_loopback(struct net *net, struct sk_buff_head *xmitq);
+int tipc_attach_loopback(struct net *net);
+void tipc_detach_loopback(struct net *net);
 
 /* check if device MTU is too low for tipc headers */
 static inline bool tipc_mtu_bad(struct net_device *dev, unsigned int reserve)
diff --git a/net/tipc/core.c b/net/tipc/core.c
index ed536c0..1867687 100644
--- a/net/tipc/core.c
+++ b/net/tipc/core.c
@@ -81,7 +81,9 @@ static int __net_init tipc_init_net(struct net *net)
 	err = tipc_bcast_init(net);
 	if (err)
 		goto out_bclink;
-
+	err = tipc_attach_loopback(net);
+	if (err)
+		goto out_bclink;
 	return 0;
 
 out_bclink:
@@ -94,6 +96,7 @@ static int __net_init tipc_init_net(struct net *net)
 
 static void __net_exit tipc_exit_net(struct net *net)
 {
+	tipc_detach_loopback(net);
 	tipc_net_stop(net);
 	tipc_bcast_stop(net);
 	tipc_nametbl_stop(net);
diff --git a/net/tipc/core.h b/net/tipc/core.h
index 7a68e1b..c1c2906 100644
--- a/net/tipc/core.h
+++ b/net/tipc/core.h
@@ -67,6 +67,7 @@ struct tipc_link;
 struct tipc_name_table;
 struct tipc_topsrv;
 struct tipc_monitor;
+void tipc_clone_to_loopback(struct net *net, struct sk_buff_head *pkts);
 
 #define TIPC_MOD_VER "2.0.0"
 
@@ -125,6 +126,10 @@ struct tipc_net {
 
 	/* Cluster capabilities */
 	u16 capabilities;
+
+	/* Tracing of node internal messages */
+	struct packet_type loopback_pt;
+	bool loopback_trace;
 };
 
 static inline struct tipc_net *tipc_net(struct net *net)
@@ -152,6 +157,13 @@ static inline struct tipc_topsrv *tipc_topsrv(struct net *net)
 	return tipc_net(net)->topsrv;
 }
 
+static inline void tipc_loopback_trace(struct net *net,
+				       struct sk_buff_head *pkts)
+{
+	if (unlikely(tipc_net(net)->loopback_trace))
+		tipc_clone_to_loopback(net, pkts);
+}
+
 static inline unsigned int tipc_hashfn(u32 addr)
 {
 	return addr & (NODE_HTABLE_SIZE - 1);
diff --git a/net/tipc/node.c b/net/tipc/node.c
index 9e106d3..7e58831 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -1439,6 +1439,7 @@ int tipc_node_xmit(struct net *net, struct sk_buff_head *list,
 	int rc;
 
 	if (in_own_node(net, dnode)) {
+		tipc_loopback_trace(net, list);
 		tipc_sk_rcv(net, list);
 		return 0;
 	}
diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index f345662..e3a6ba1 100644
--- a/net/tipc/topsrv.c
+++ b/net/tipc/topsrv.c
@@ -40,6 +40,7 @@
 #include "socket.h"
 #include "addr.h"
 #include "msg.h"
+#include "bearer.h"
 #include <net/sock.h>
 #include <linux/module.h>
 
@@ -608,6 +609,7 @@ static void tipc_topsrv_kern_evt(struct net *net, struct tipc_event *evt)
 	memcpy(msg_data(buf_msg(skb)), evt, sizeof(*evt));
 	skb_queue_head_init(&evtq);
 	__skb_queue_tail(&evtq, skb);
+	tipc_loopback_trace(net, &evtq);
 	tipc_sk_rcv(net, &evtq);
 }
 
-- 
2.11.0

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

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

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 <yin...@wi...>
>> Sent: 21-Jun-19 08:16
>> To: John Rutherford <joh...@de...>; tipc-
>> dis...@li...
>> 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 <joh...@de...>
>>> ---
>>>  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, &localq);
>>>  		tipc_sk_mcast_rcv(net, &localq, &inputq);
>>> +	}
>>>  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(&tn->loopback_pt);
>>> +	return 0;
>>> +}
>>> +
>>> +void tipc_detach_loopback(struct net *net) {
>>> +	struct tipc_net *tn = tipc_net(net);
>>> +
>>> +	dev_remove_pack(&tn->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,
>>>  		      struct tipc_media_addr *dst);  void
>>> tipc_bearer_bc_xmit(struct net *net, u32 bearer_id,
>>>  			 struct sk_buff_head *xmitq);
>>> +void tipc_clone_to_loopback(struct net *net, struct sk_buff_head
>>> +*xmitq); int tipc_attach_loopback(struct net *net); void
>>> +tipc_detach_loopback(struct net *net);
>>>
>>>  /* check if device MTU is too low for tipc headers */  static inline
>>> bool tipc_mtu_bad(struct net_device *dev, unsigned int reserve) diff
>>> --git a/net/tipc/core.c b/net/tipc/core.c index ed536c0..1867687
>>> 100644
>>> --- a/net/tipc/core.c
>>> +++ b/net/tipc/core.c
>>> @@ -81,7 +81,9 @@ static int __net_init tipc_init_net(struct net *net)
>>>  	err = tipc_bcast_init(net);
>>>  	if (err)
>>>  		goto out_bclink;
>>> -
>>> +	err = tipc_attach_loopback(net);
>>> +	if (err)
>>> +		goto out_bclink;
>>>  	return 0;
>>>
>>>  out_bclink:
>>> @@ -94,6 +96,7 @@ static int __net_init tipc_init_net(struct net *net)
>>>
>>>  static void __net_exit tipc_exit_net(struct net *net)  {
>>> +	tipc_detach_loopback(net);
>>>  	tipc_net_stop(net);
>>>  	tipc_bcast_stop(net);
>>>  	tipc_nametbl_stop(net);
>>> diff --git a/net/tipc/core.h b/net/tipc/core.h index 7a68e1b..c1c2906
>>> 100644
>>> --- a/net/tipc/core.h
>>> +++ b/net/tipc/core.h
>>> @@ -67,6 +67,7 @@ struct tipc_link;
>>>  struct tipc_name_table;
>>>  struct tipc_topsrv;
>>>  struct tipc_monitor;
>>> +void tipc_clone_to_loopback(struct net *net, struct sk_buff_head
>>> +*pkts);
>>>
>>>  #define TIPC_MOD_VER "2.0.0"
>>>
>>> @@ -125,6 +126,10 @@ struct tipc_net {
>>>
>>>  	/* Cluster capabilities */
>>>  	u16 capabilities;
>>> +
>>> +	/* Tracing of node internal messages */
>>> +	struct packet_type loopback_pt;
>>> +	bool loopback_trace;
>>>  };
>>>
>>>  static inline struct tipc_net *tipc_net(struct net *net) @@ -152,6
>>> +157,13 @@ static inline struct tipc_topsrv *tipc_topsrv(struct net *net)
>>>  	return tipc_net(net)->topsrv;
>>>  }
>>>
>>> +static inline void tipc_loopback_trace(struct net *net,
>>> +				       struct sk_buff_head *pkts) {
>>> +	if (unlikely(tipc_net(net)->loopback_trace))
>>> +		tipc_clone_to_loopback(net, pkts);
>>> +}
>>> +
>>>  static inline unsigned int tipc_hashfn(u32 addr)  {
>>>  	return addr & (NODE_HTABLE_SIZE - 1); diff --git a/net/tipc/node.c
>>> b/net/tipc/node.c index 9e106d3..7e58831 100644
>>> --- a/net/tipc/node.c
>>> +++ b/net/tipc/node.c
>>> @@ -1439,6 +1439,7 @@ int tipc_node_xmit(struct net *net, struct
>> sk_buff_head *list,
>>>  	int rc;
>>>
>>>  	if (in_own_node(net, dnode)) {
>>> +		tipc_loopback_trace(net, list);
>>>  		tipc_sk_rcv(net, list);
>>>  		return 0;
>>>  	}
>>> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c index
>>> f345662..e3a6ba1 100644
>>> --- a/net/tipc/topsrv.c
>>> +++ b/net/tipc/topsrv.c
>>> @@ -40,6 +40,7 @@
>>>  #include "socket.h"
>>>  #include "addr.h"
>>>  #include "msg.h"
>>> +#include "bearer.h"
>>>  #include <net/sock.h>
>>>  #include <linux/module.h>
>>>
>>> @@ -608,6 +609,7 @@ static void tipc_topsrv_kern_evt(struct net *net,
>> struct tipc_event *evt)
>>>  	memcpy(msg_data(buf_msg(skb)), evt, sizeof(*evt));
>>>  	skb_queue_head_init(&evtq);
>>>  	__skb_queue_tail(&evtq, skb);
>>> +	tipc_loopback_trace(net, &evtq);
>>>  	tipc_sk_rcv(net, &evtq);
>>>  }
>>>
>>>
>>
>>
>> _______________________________________________
>> tipc-discussion mailing list
>> tip...@li...
>> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
> 

Re: [tipc-discussion] [iproute2-next v5] tipc: support interface name when activating UDP bearer

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

Thanks David. I will update code change as your comments.

For the item:
> +	/* remove from cache */
> +	ll_drop_by_index(ifr.ifr_ifindex);

why the call to ll_drop_by_index? doing so means that ifindex is looked
up again.

[Hoang]
> +	ifr.ifr_ifindex = ll_name_to_index(ifr.ifr_name);
This function stored an entry ll_cache in hash map table. We have to call this function to prevent memory leaked. 

Regards,
Hoang
-----Original Message-----
From: David Ahern <ds...@gm...> 
Sent: Saturday, June 22, 2019 5:50 AM
To: Hoang Le <hoa...@de...>; ds...@gm...; jon...@er...; ma...@do...; yin...@wi...; ne...@vg...; tip...@li...
Subject: Re: [iproute2-next v5] tipc: support interface name when activating UDP bearer

On 6/13/19 2:07 AM, Hoang Le wrote:
> @@ -119,6 +121,74 @@ static int generate_multicast(short af, char *buf, int bufsize)
>  	return 0;
>  }
>  
> +static struct ifreq ifr = {};

you don't need to initialize globals, but you could pass a a struct as
the arg to the filter here which is both the addr buffer and the ifindex
of interest.

> +static int nl_dump_addr_filter(struct nlmsghdr *nlh, void *arg)
> +{
> +	struct ifaddrmsg *ifa = NLMSG_DATA(nlh);
> +	char *r_addr = (char *)arg;
> +	int len = nlh->nlmsg_len;
> +	struct rtattr *addr_attr;
> +
> +	if (ifr.ifr_ifindex != ifa->ifa_index)
> +		return 0;
> +
> +	if (strlen(r_addr) > 0)
> +		return 1;
> +
> +	addr_attr = parse_rtattr_one(IFA_ADDRESS, IFA_RTA(ifa),
> +				     len - NLMSG_LENGTH(sizeof(*ifa)));
> +	if (!addr_attr)
> +		return 0;
> +
> +	if (ifa->ifa_family == AF_INET) {
> +		struct sockaddr_in ip4addr;
> +		memcpy(&ip4addr.sin_addr, RTA_DATA(addr_attr),
> +		       sizeof(struct in_addr));
> +		if (inet_ntop(AF_INET, &ip4addr.sin_addr, r_addr,
> +			      INET_ADDRSTRLEN) == NULL)
> +			return 0;
> +	} else if (ifa->ifa_family == AF_INET6) {
> +		struct sockaddr_in6 ip6addr;
> +		memcpy(&ip6addr.sin6_addr, RTA_DATA(addr_attr),
> +		       sizeof(struct in6_addr));
> +		if (inet_ntop(AF_INET6, &ip6addr.sin6_addr, r_addr,
> +			      INET6_ADDRSTRLEN) == NULL)
> +			return 0;
> +	}
> +	return 1;
> +}
> +
> +static int cmd_bearer_validate_and_get_addr(const char *name, char *r_addr)
> +{
> +	struct rtnl_handle rth ={ .fd = -1 };

space between '={'

> +
> +	memset(&ifr, 0, sizeof(ifr));
> +	if (!name || !r_addr || get_ifname(ifr.ifr_name, name))
> +		return 0;
> +
> +	ifr.ifr_ifindex = ll_name_to_index(ifr.ifr_name);
> +	if (!ifr.ifr_ifindex)
> +		return 0;
> +
> +	/* remove from cache */
> +	ll_drop_by_index(ifr.ifr_ifindex);

why the call to ll_drop_by_index? doing so means that ifindex is looked
up again.

> +
> +	if (rtnl_open(&rth, 0) < 0)
> +		return 0;
> +
> +	if (rtnl_addrdump_req(&rth, AF_UNSPEC, 0) < 0) {

If you pass a filter here to set ifa_index, this command on newer
kernels will be much more efficient. See ipaddr_dump_filter.


> +		rtnl_close(&rth);
> +		return 0;
> +	}
> +
> +	if (rtnl_dump_filter(&rth, nl_dump_addr_filter, r_addr) < 0) {
> +		rtnl_close(&rth);
> +		return 0;
> +	}
> +	rtnl_close(&rth);
> +	return 1;
> +}

it would better to have 1 exit with the rtnl_close and return rc based
on above.

Re: [tipc-discussion] [PATCH net] tipc: add dst_cache support for udp media

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

From: Xin Long <luc...@gm...>
Date: Thu, 20 Jun 2019 19:03:41 +0800

> As other udp/ip tunnels do, tipc udp media should also have a
> lockless dst_cache supported on its tx path.
> 
> Here we add dst_cache into udp_replicast to support dst cache
> for both rmcast and rcast, and rmcast uses ub->rcast and each
> rcast uses its own node in ub->rcast.list.
> 
> Signed-off-by: Xin Long <luc...@gm...>

I'll apply this to net-next after the next net --> net-next
marge since it depends upon the register_pernet_device change.

Re: [tipc-discussion] [PATCH net] tipc: change to use register_pernet_device

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

From: Xin Long <luc...@gm...>
Date: Thu, 20 Jun 2019 18:39:28 +0800

> This patch is to fix a dst defcnt leak, which can be reproduced by doing:
> 
>   # ip net a c; ip net a s; modprobe tipc
>   # ip net e s ip l a n eth1 type veth peer n eth1 netns c
>   # ip net e c ip l s lo up; ip net e c ip l s eth1 up
>   # ip net e s ip l s lo up; ip net e s ip l s eth1 up
>   # ip net e c ip a a 1.1.1.2/8 dev eth1
>   # ip net e s ip a a 1.1.1.1/8 dev eth1
>   # ip net e c tipc b e m udp n u1 localip 1.1.1.2
>   # ip net e s tipc b e m udp n u1 localip 1.1.1.1
>   # ip net d c; ip net d s; rmmod tipc
> 
> and it will get stuck and keep logging the error:
> 
>   unregister_netdevice: waiting for lo to become free. Usage count = 1
> 
> The cause is that a dst is held by the udp sock's sk_rx_dst set on udp rx
> path with udp_early_demux == 1, and this dst (eventually holding lo dev)
> can't be released as bearer's removal in tipc pernet .exit happens after
> lo dev's removal, default_device pernet .exit.
> 
>  "There are two distinct types of pernet_operations recognized: subsys and
>   device.  At creation all subsys init functions are called before device
>   init functions, and at destruction all device exit functions are called
>   before subsys exit function."
> 
> So by calling register_pernet_device instead to register tipc_net_ops, the
> pernet .exit() will be invoked earlier than loopback dev's removal when a
> netns is being destroyed, as fou/gue does.
> 
> Note that vxlan and geneve udp tunnels don't have this issue, as the udp
> sock is released in their device ndo_stop().
> 
> This fix is also necessary for tipc dst_cache, which will hold dsts on tx
> path and I will introduce in my next patch.
> 
> Reported-by: Li Shuang <sh...@re...>
> Signed-off-by: Xin Long <luc...@gm...>

Applied.

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

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

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?

///jon


> -----Original Message-----
> From: Ying Xue <yin...@wi...>
> Sent: 21-Jun-19 08:16
> To: John Rutherford <joh...@de...>; tipc-
> dis...@li...
> 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 <joh...@de...>
> > ---
> >  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, &localq);
> >  		tipc_sk_mcast_rcv(net, &localq, &inputq);
> > +	}
> >  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(&tn->loopback_pt);
> > +	return 0;
> > +}
> > +
> > +void tipc_detach_loopback(struct net *net) {
> > +	struct tipc_net *tn = tipc_net(net);
> > +
> > +	dev_remove_pack(&tn->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,
> >  		      struct tipc_media_addr *dst);  void
> > tipc_bearer_bc_xmit(struct net *net, u32 bearer_id,
> >  			 struct sk_buff_head *xmitq);
> > +void tipc_clone_to_loopback(struct net *net, struct sk_buff_head
> > +*xmitq); int tipc_attach_loopback(struct net *net); void
> > +tipc_detach_loopback(struct net *net);
> >
> >  /* check if device MTU is too low for tipc headers */  static inline
> > bool tipc_mtu_bad(struct net_device *dev, unsigned int reserve) diff
> > --git a/net/tipc/core.c b/net/tipc/core.c index ed536c0..1867687
> > 100644
> > --- a/net/tipc/core.c
> > +++ b/net/tipc/core.c
> > @@ -81,7 +81,9 @@ static int __net_init tipc_init_net(struct net *net)
> >  	err = tipc_bcast_init(net);
> >  	if (err)
> >  		goto out_bclink;
> > -
> > +	err = tipc_attach_loopback(net);
> > +	if (err)
> > +		goto out_bclink;
> >  	return 0;
> >
> >  out_bclink:
> > @@ -94,6 +96,7 @@ static int __net_init tipc_init_net(struct net *net)
> >
> >  static void __net_exit tipc_exit_net(struct net *net)  {
> > +	tipc_detach_loopback(net);
> >  	tipc_net_stop(net);
> >  	tipc_bcast_stop(net);
> >  	tipc_nametbl_stop(net);
> > diff --git a/net/tipc/core.h b/net/tipc/core.h index 7a68e1b..c1c2906
> > 100644
> > --- a/net/tipc/core.h
> > +++ b/net/tipc/core.h
> > @@ -67,6 +67,7 @@ struct tipc_link;
> >  struct tipc_name_table;
> >  struct tipc_topsrv;
> >  struct tipc_monitor;
> > +void tipc_clone_to_loopback(struct net *net, struct sk_buff_head
> > +*pkts);
> >
> >  #define TIPC_MOD_VER "2.0.0"
> >
> > @@ -125,6 +126,10 @@ struct tipc_net {
> >
> >  	/* Cluster capabilities */
> >  	u16 capabilities;
> > +
> > +	/* Tracing of node internal messages */
> > +	struct packet_type loopback_pt;
> > +	bool loopback_trace;
> >  };
> >
> >  static inline struct tipc_net *tipc_net(struct net *net) @@ -152,6
> > +157,13 @@ static inline struct tipc_topsrv *tipc_topsrv(struct net *net)
> >  	return tipc_net(net)->topsrv;
> >  }
> >
> > +static inline void tipc_loopback_trace(struct net *net,
> > +				       struct sk_buff_head *pkts) {
> > +	if (unlikely(tipc_net(net)->loopback_trace))
> > +		tipc_clone_to_loopback(net, pkts);
> > +}
> > +
> >  static inline unsigned int tipc_hashfn(u32 addr)  {
> >  	return addr & (NODE_HTABLE_SIZE - 1); diff --git a/net/tipc/node.c
> > b/net/tipc/node.c index 9e106d3..7e58831 100644
> > --- a/net/tipc/node.c
> > +++ b/net/tipc/node.c
> > @@ -1439,6 +1439,7 @@ int tipc_node_xmit(struct net *net, struct
> sk_buff_head *list,
> >  	int rc;
> >
> >  	if (in_own_node(net, dnode)) {
> > +		tipc_loopback_trace(net, list);
> >  		tipc_sk_rcv(net, list);
> >  		return 0;
> >  	}
> > diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c index
> > f345662..e3a6ba1 100644
> > --- a/net/tipc/topsrv.c
> > +++ b/net/tipc/topsrv.c
> > @@ -40,6 +40,7 @@
> >  #include "socket.h"
> >  #include "addr.h"
> >  #include "msg.h"
> > +#include "bearer.h"
> >  #include <net/sock.h>
> >  #include <linux/module.h>
> >
> > @@ -608,6 +609,7 @@ static void tipc_topsrv_kern_evt(struct net *net,
> struct tipc_event *evt)
> >  	memcpy(msg_data(buf_msg(skb)), evt, sizeof(*evt));
> >  	skb_queue_head_init(&evtq);
> >  	__skb_queue_tail(&evtq, skb);
> > +	tipc_loopback_trace(net, &evtq);
> >  	tipc_sk_rcv(net, &evtq);
> >  }
> >
> >
> 
> 
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion

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

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

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 <joh...@de...>
> ---
>  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, &localq);
>  		tipc_sk_mcast_rcv(net, &localq, &inputq);
> +	}
>  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(&tn->loopback_pt);
> +	return 0;
> +}
> +
> +void tipc_detach_loopback(struct net *net)
> +{
> +	struct tipc_net *tn = tipc_net(net);
> +
> +	dev_remove_pack(&tn->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,
>  		      struct tipc_media_addr *dst);
>  void tipc_bearer_bc_xmit(struct net *net, u32 bearer_id,
>  			 struct sk_buff_head *xmitq);
> +void tipc_clone_to_loopback(struct net *net, struct sk_buff_head *xmitq);
> +int tipc_attach_loopback(struct net *net);
> +void tipc_detach_loopback(struct net *net);
>  
>  /* check if device MTU is too low for tipc headers */
>  static inline bool tipc_mtu_bad(struct net_device *dev, unsigned int reserve)
> diff --git a/net/tipc/core.c b/net/tipc/core.c
> index ed536c0..1867687 100644
> --- a/net/tipc/core.c
> +++ b/net/tipc/core.c
> @@ -81,7 +81,9 @@ static int __net_init tipc_init_net(struct net *net)
>  	err = tipc_bcast_init(net);
>  	if (err)
>  		goto out_bclink;
> -
> +	err = tipc_attach_loopback(net);
> +	if (err)
> +		goto out_bclink;
>  	return 0;
>  
>  out_bclink:
> @@ -94,6 +96,7 @@ static int __net_init tipc_init_net(struct net *net)
>  
>  static void __net_exit tipc_exit_net(struct net *net)
>  {
> +	tipc_detach_loopback(net);
>  	tipc_net_stop(net);
>  	tipc_bcast_stop(net);
>  	tipc_nametbl_stop(net);
> diff --git a/net/tipc/core.h b/net/tipc/core.h
> index 7a68e1b..c1c2906 100644
> --- a/net/tipc/core.h
> +++ b/net/tipc/core.h
> @@ -67,6 +67,7 @@ struct tipc_link;
>  struct tipc_name_table;
>  struct tipc_topsrv;
>  struct tipc_monitor;
> +void tipc_clone_to_loopback(struct net *net, struct sk_buff_head *pkts);
>  
>  #define TIPC_MOD_VER "2.0.0"
>  
> @@ -125,6 +126,10 @@ struct tipc_net {
>  
>  	/* Cluster capabilities */
>  	u16 capabilities;
> +
> +	/* Tracing of node internal messages */
> +	struct packet_type loopback_pt;
> +	bool loopback_trace;
>  };
>  
>  static inline struct tipc_net *tipc_net(struct net *net)
> @@ -152,6 +157,13 @@ static inline struct tipc_topsrv *tipc_topsrv(struct net *net)
>  	return tipc_net(net)->topsrv;
>  }
>  
> +static inline void tipc_loopback_trace(struct net *net,
> +				       struct sk_buff_head *pkts)
> +{
> +	if (unlikely(tipc_net(net)->loopback_trace))
> +		tipc_clone_to_loopback(net, pkts);
> +}
> +
>  static inline unsigned int tipc_hashfn(u32 addr)
>  {
>  	return addr & (NODE_HTABLE_SIZE - 1);
> diff --git a/net/tipc/node.c b/net/tipc/node.c
> index 9e106d3..7e58831 100644
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -1439,6 +1439,7 @@ int tipc_node_xmit(struct net *net, struct sk_buff_head *list,
>  	int rc;
>  
>  	if (in_own_node(net, dnode)) {
> +		tipc_loopback_trace(net, list);
>  		tipc_sk_rcv(net, list);
>  		return 0;
>  	}
> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index f345662..e3a6ba1 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c
> @@ -40,6 +40,7 @@
>  #include "socket.h"
>  #include "addr.h"
>  #include "msg.h"
> +#include "bearer.h"
>  #include <net/sock.h>
>  #include <linux/module.h>
>  
> @@ -608,6 +609,7 @@ static void tipc_topsrv_kern_evt(struct net *net, struct tipc_event *evt)
>  	memcpy(msg_data(buf_msg(skb)), evt, sizeof(*evt));
>  	skb_queue_head_init(&evtq);
>  	__skb_queue_tail(&evtq, skb);
> +	tipc_loopback_trace(net, &evtq);
>  	tipc_sk_rcv(net, &evtq);
>  }
>  
> 

Re: [tipc-discussion] KMSAN: uninit-value in tipc_nl_compat_bearer_disable

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

On Wed, Jun 19, 2019 at 11:48 PM syzbot
<syz...@sy...> wrote:
>
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit:    f75e4cfe kmsan: use kmsan_handle_urb() in urb.c
> git tree:       kmsan
> console output: https://syzkaller.appspot.com/x/log.txt?x=13d0a6fea00000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=602468164ccdc30a
> dashboard link: https://syzkaller.appspot.com/bug?extid=30eaa8bf392f7fafffaf
> compiler:       clang version 9.0.0 (/home/glider/llvm/clang
> 06d00afa61eef8f7f501ebdb4e8612ea43ec2d78)
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=15b4a95aa00000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=162fc761a00000
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syz...@sy...
>
> IPv6: ADDRCONF(NETDEV_CHANGE): hsr0: link becomes ready
> 8021q: adding VLAN 0 to HW filter on device batadv0
> ==================================================================
> BUG: KMSAN: uninit-value in memchr+0xce/0x110 lib/string.c:981
> CPU: 0 PID: 12554 Comm: syz-executor731 Not tainted 5.1.0+ #1
> 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+0x191/0x1f0 lib/dump_stack.c:113
>   kmsan_report+0x130/0x2a0 mm/kmsan/kmsan.c:622
>   __msan_warning+0x75/0xe0 mm/kmsan/kmsan_instr.c:310
>   memchr+0xce/0x110 lib/string.c:981
>   string_is_valid net/tipc/netlink_compat.c:176 [inline]
>   tipc_nl_compat_bearer_disable+0x2a1/0x480 net/tipc/netlink_compat.c:449
TLV_GET_DATA_LEN(msg->req) may return a negtive value, which will be
used as size_t (a big unsigned long) passed into  memchr(),
triggered this issue.

@@ -446,7 +446,7 @@ static int tipc_nl_compat_bearer_disable(struct
tipc_nl_compat_cmd_doit *cmd,
  return -EMSGSIZE;

  len = min_t(int, TLV_GET_DATA_LEN(msg->req), TIPC_MAX_BEARER_NAME);
- if (!string_is_valid(name, len))
+ if (len <=0 || !string_is_valid(name, len))
  return -EINVAL;

The same fix is needed for some other places, and I will give a fix-ups.

>   __tipc_nl_compat_doit net/tipc/netlink_compat.c:327 [inline]
>   tipc_nl_compat_doit+0x3ac/0xb00 net/tipc/netlink_compat.c:360
>   tipc_nl_compat_handle net/tipc/netlink_compat.c:1178 [inline]
>   tipc_nl_compat_recv+0x1b1b/0x27b0 net/tipc/netlink_compat.c:1281
>   genl_family_rcv_msg net/netlink/genetlink.c:602 [inline]
>   genl_rcv_msg+0x185a/0x1a40 net/netlink/genetlink.c:627
>   netlink_rcv_skb+0x431/0x620 net/netlink/af_netlink.c:2486
>   genl_rcv+0x63/0x80 net/netlink/genetlink.c:638
>   netlink_unicast_kernel net/netlink/af_netlink.c:1311 [inline]
>   netlink_unicast+0xf3e/0x1020 net/netlink/af_netlink.c:1337
>   netlink_sendmsg+0x127e/0x12f0 net/netlink/af_netlink.c:1926
>   sock_sendmsg_nosec net/socket.c:651 [inline]
>   sock_sendmsg net/socket.c:661 [inline]
>   ___sys_sendmsg+0xcc6/0x1200 net/socket.c:2260
>   __sys_sendmsg net/socket.c:2298 [inline]
>   __do_sys_sendmsg net/socket.c:2307 [inline]
>   __se_sys_sendmsg+0x305/0x460 net/socket.c:2305
>   __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2305
>   do_syscall_64+0xbc/0xf0 arch/x86/entry/common.c:291
>   entry_SYSCALL_64_after_hwframe+0x63/0xe7
> RIP: 0033:0x442639
> Code: 41 02 00 85 c0 b8 00 00 00 00 48 0f 44 c3 5b c3 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 fb 10 fc ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:00000000007efea8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000442639
> RDX: 0000000000000000 RSI: 0000000020000080 RDI: 0000000000000003
> RBP: 00000000007eff00 R08: 0000000000000003 R09: 0000000000000003
> R10: 00000000bb1414ac R11: 0000000000000246 R12: 0000000000000003
> R13: 0000000000403c50 R14: 0000000000000000 R15: 0000000000000000
>
> Uninit was created at:
>   kmsan_save_stack_with_flags mm/kmsan/kmsan.c:208 [inline]
>   kmsan_internal_poison_shadow+0x92/0x150 mm/kmsan/kmsan.c:162
>   kmsan_kmalloc+0xa4/0x130 mm/kmsan/kmsan_hooks.c:175
>   kmsan_slab_alloc+0xe/0x10 mm/kmsan/kmsan_hooks.c:184
>   slab_post_alloc_hook mm/slab.h:442 [inline]
>   slab_alloc_node mm/slub.c:2771 [inline]
>   __kmalloc_node_track_caller+0xcba/0xf30 mm/slub.c:4399
>   __kmalloc_reserve net/core/skbuff.c:140 [inline]
>   __alloc_skb+0x306/0xa10 net/core/skbuff.c:208
>   alloc_skb include/linux/skbuff.h:1059 [inline]
>   netlink_alloc_large_skb net/netlink/af_netlink.c:1183 [inline]
>   netlink_sendmsg+0xb81/0x12f0 net/netlink/af_netlink.c:1901
>   sock_sendmsg_nosec net/socket.c:651 [inline]
>   sock_sendmsg net/socket.c:661 [inline]
>   ___sys_sendmsg+0xcc6/0x1200 net/socket.c:2260
>   __sys_sendmsg net/socket.c:2298 [inline]
>   __do_sys_sendmsg net/socket.c:2307 [inline]
>   __se_sys_sendmsg+0x305/0x460 net/socket.c:2305
>   __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2305
>   do_syscall_64+0xbc/0xf0 arch/x86/entry/common.c:291
>   entry_SYSCALL_64_after_hwframe+0x63/0xe7
> ==================================================================
>
>
> ---
> This bug is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syz...@go....
>
> syzbot will keep track of this bug report. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
> syzbot can test patches for this bug, for details see:
> https://goo.gl/tpsmEJ#testing-patches

Re: [tipc-discussion] [PATCH net 0/3] net: fix quite a few dst_cache crashes reported by syzbot

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

On Fri, Jun 21, 2019 at 3:48 AM Erik Hugne <eri...@gm...> wrote:
>
> Commenting on Jon's response fist.
>
> Den tors 20 juni 2019 kl 13:26 skrev Xin Long <luc...@gm...>:
> >
> > On Mon, Jun 17, 2019 at 10:28 PM Jon Maloy <jon...@er...> wrote:
> > > Hi Xin,
> > > As I remember the discussion around introduction of UDP media a few years ago, the developer, Erik Huge, only chose to register TIPC as a udp tunnel user instead of regular udp user because it provides a more efficient way to receive packet in kernel space.
> > >With UDP tunnel, we could receive packet directly in a callback, while TIPC had to run in a work queue thread in order to read packets from the socket.
> The performance was largely dependant on TIPC message size, for large packets there was no measurable difference, but the tunnel approach was considerably faster for small packets than the kernel socket interface.
> I dont have the numbers, but i think i posted them on this list around 8 years ago.
>
> >[...]
> > To implement this gso callback, we need to require an ipproto number for TIPC,
> > and register the callback into inet_offloads by inet_add_offload().
> > And on tx path set:
> > skb->encapsulation = 1,
> > skb_shinfo(skb)->gso_type|= SKB_GSO_UDP_TUNNEL,
> > skb->inner_protocol_type = ENCAP_TYPE_IPPROTO.
> >
> > Then it will be called by:
> > dev_queue_xmit() .. -> skb_mac_gso_segment() ... ->
> > udp4_ufo_fragment() -> skb_udp_tunnel_segment() ->
> > skb_udp_tunnel_segment() -> tipc_gso_fragment()
> >
> > btw, do we have an official ipproto number for TIPC already?
>
> Not afak, but we have an IANA assigned UDP port for TIPC though, 6118.
> https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xhtml?search=tipc
>
> TIPC does:
>         skb_set_inner_protocol(skb, htons(ETH_P_TIPC));
> which will in turn set skb_inner_protocol_type to ENCAP_TYPE_ETHER.
> So how about implementing something similar to what's done for ENCAP_TYPE_IPPROTO, but for ENCAP_TYPE_ETHER?
>
> In udp_offload.c, something in the line of:
>
> ...
> skb_udp_tunnel_segment(....)
> ....
>
> switch (skb->inner_protocol_type) {
>     case ENCAP_TYPE_ETHER:
>         protocol = skb->inner_protocol;
>         ops = rcu_dereference(ether_offloads[protocol]);
>         if (!ops || !ops->callbacks->gso_segment)
>             goto out_unlock;
>         gso_inner_segment = ops->callbacks.gso_segment;
>     break;
>
> ....
> And obviously define ether_offloads, and corresponding ether_add_protocol and ether_add_offload functions.
>
Maybe no need ether_offloads, dev_add_offload(&tipc_packet_offload) is
enough to make the callback be called by skb_mac_gso_segment() from
skb_udp_tunnel_segment(), I believe that's also what Jon does now.

It depends on which layer protocol we think TIPC. If we don't have a
plan for TIPC working over IP, a transport protocol in the future,
packet_offload is fine, otherwise, inet_offloads is also an option.

Re: [tipc-discussion] [PATCH net 0/3] net: fix quite a few dst_cache crashes reported by syzbot

[Erik H. <eri...@gm...>]

Commenting on Jon's response fist.

Den tors 20 juni 2019 kl 13:26 skrev Xin Long <luc...@gm...>:
>
> On Mon, Jun 17, 2019 at 10:28 PM Jon Maloy <jon...@er...> wrote:
> > Hi Xin,
> > As I remember the discussion around introduction of UDP media a few
years ago, the developer, Erik Huge, only chose to register TIPC as a udp
tunnel user instead of regular udp user because it provides a more
efficient way to receive packet in kernel space.
> >With UDP tunnel, we could receive packet directly in a callback, while
TIPC had to run in a work queue thread in order to read packets from the
socket.
The performance was largely dependant on TIPC message size, for large
packets there was no measurable difference, but the tunnel approach was
considerably faster for small packets than the kernel socket interface.
I dont have the numbers, but i think i posted them on this list around 8
years ago.

>[...]
> To implement this gso callback, we need to require an ipproto number for
TIPC,
> and register the callback into inet_offloads by inet_add_offload().
> And on tx path set:
> skb->encapsulation = 1,
> skb_shinfo(skb)->gso_type|= SKB_GSO_UDP_TUNNEL,
> skb->inner_protocol_type = ENCAP_TYPE_IPPROTO.
>
> Then it will be called by:
> dev_queue_xmit() .. -> skb_mac_gso_segment() ... ->
> udp4_ufo_fragment() -> skb_udp_tunnel_segment() ->
> skb_udp_tunnel_segment() -> tipc_gso_fragment()
>
> btw, do we have an official ipproto number for TIPC already?

Not afak, but we have an IANA assigned UDP port for TIPC though, 6118.
https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xhtml?search=tipc

TIPC does:
        skb_set_inner_protocol
<https://elixir.bootlin.com/linux/v5.2-rc5/ident/skb_set_inner_protocol>(skb
, htons <https://elixir.bootlin.com/linux/v5.2-rc5/ident/htons>(ETH_P_TIPC
<https://elixir.bootlin.com/linux/v5.2-rc5/ident/ETH_P_TIPC>));
which will in turn set skb_inner_protocol_type to ENCAP_TYPE_ETHER.
So how about implementing something similar to what's done for
ENCAP_TYPE_IPPROTO, but for ENCAP_TYPE_ETHER?

In udp_offload.c, something in the line of:

...
skb_udp_tunnel_segment(....)
....

switch (skb->inner_protocol_type) {
    case ENCAP_TYPE_ETHER:
        protocol = skb->inner_protocol;
        ops = rcu_dereference(ether_offloads[protocol]);
        if (!ops || !ops->callbacks->gso_segment)
            goto out_unlock;
        gso_inner_segment = ops->callbacks.gso_segment;
    break;

....
And obviously define ether_offloads, and corresponding ether_add_protocol
and ether_add_offload functions.

//E

Re: [tipc-discussion] [PATCH net 0/3] net: fix quite a few dst_cache crashes reported by syzbot

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

> -----Original Message-----
> From: Xin Long <luc...@gm...>
> Sent: 20-Jun-19 07:26
> To: Jon Maloy <jon...@er...>
> Cc: tip...@li...
> Subject: Re: [PATCH net 0/3] net: fix quite a few dst_cache crashes reported
> by syzbot
> 
> On Mon, Jun 17, 2019 at 10:28 PM Jon Maloy <jon...@er...>
> wrote:
> >
> > Hi Xin,
> > As I remember the discussion around introduction of UDP media a few years
> ago, the developer, Erik Huge, only chose to register TIPC as a udp tunnel user
> instead of regular udp user because it provides a more efficient way to receive
> packet in kernel space.
> > With UDP tunnel, we could receive packet directly in a callback, while TIPC
> had to run in a work queue thread in order to read packets from the socket.
> So, in reality we don't need any tunnel at all. Another upside is that it is
> possible to hook in a GSO callback function from the tunnel user, something I
> am uncertain if we can do as a regular UDP user.
> 
> Right, udp tunnel was invented for this kind of encapsulation.
> 
> To implement this gso callback, we need to require an ipproto number for
> TIPC, and register the callback into inet_offloads by inet_add_offload().
> And on tx path set:
> skb->encapsulation = 1,
> skb_shinfo(skb)->gso_type|= SKB_GSO_UDP_TUNNEL,
> skb->inner_protocol_type = ENCAP_TYPE_IPPROTO.
> 
> Then it will be called by:
> dev_queue_xmit() .. -> skb_mac_gso_segment() ... ->
> udp4_ufo_fragment() -> skb_udp_tunnel_segment() ->
> skb_udp_tunnel_segment() -> tipc_gso_fragment()

This has already been done.

> 
> btw, do we have an official ipproto number for TIPC already?

No, I didn't push for this, since I was uncertain if it would be needed.

I did an experiment with this, as follows:
- I defined a new IPPROTO_TIPC and added it to the relevant locations in the network stack.
- I created a raw socket and sent packets from that via the ip_build_and_send_pkt() function.
- I registered a new tipc_ip_rcv() function via the inet_add_protocol() function.

This of course would require a new TIPC_GSO type, but we would avoid the horrendously deep call chain we currently have SKB_GSO_UDP_TUNNEL. 
I am pretty sure this was one of the reasons I was unable to improve performance this way.
I am also uncertain how a new IP protocol would be handled by existing routers and filters.

///jon


> 
> > Do you have any comments on this? Could it possibly be done differently?
> >
> > ///jon
> >
> >
> > > -----Original Message-----
> > > From: net...@vg... <net...@vg...>
> On
> > > Behalf Of Xin Long
> > > Sent: 17-Jun-19 09:34
> > > To: network dev <ne...@vg...>
> > > Cc: da...@da...; Jon Maloy <jon...@er...>; Ying
> > > Xue <yin...@wi...>; tip...@li...;
> > > Marcelo Ricardo Leitner <mar...@gm...>; Neil Horman
> > > <nh...@tu...>; Su Yanjun <suy...@cn...>; David
> > > Ahern <ds...@gm...>; syz...@go...; Dmitry
> > > Vyukov <dv...@go...>; Pravin B Shelar <ps...@ni...>
> > > Subject: [PATCH net 0/3] net: fix quite a few dst_cache crashes
> > > reported by syzbot
> > >
> > > There are two kinds of crashes reported many times by syzbot with no
> > > reproducer. Call Traces are like:
> > >
> > >      BUG: KASAN: slab-out-of-bounds in rt_cache_valid+0x158/0x190
> > >      net/ipv4/route.c:1556
> > >        rt_cache_valid+0x158/0x190 net/ipv4/route.c:1556
> > >        __mkroute_output net/ipv4/route.c:2332 [inline]
> > >        ip_route_output_key_hash_rcu+0x819/0x2d50
> net/ipv4/route.c:2564
> > >        ip_route_output_key_hash+0x1ef/0x360 net/ipv4/route.c:2393
> > >        __ip_route_output_key include/net/route.h:125 [inline]
> > >        ip_route_output_flow+0x28/0xc0 net/ipv4/route.c:2651
> > >        ip_route_output_key include/net/route.h:135 [inline]
> > >      ...
> > >
> > >    or:
> > >
> > >      kasan: GPF could be caused by NULL-ptr deref or user memory access
> > >      RIP: 0010:dst_dev_put+0x24/0x290 net/core/dst.c:168
> > >        <IRQ>
> > >        rt_fibinfo_free_cpus net/ipv4/fib_semantics.c:200 [inline]
> > >        free_fib_info_rcu+0x2e1/0x490 net/ipv4/fib_semantics.c:217
> > >        __rcu_reclaim kernel/rcu/rcu.h:240 [inline]
> > >        rcu_do_batch kernel/rcu/tree.c:2437 [inline]
> > >        invoke_rcu_callbacks kernel/rcu/tree.c:2716 [inline]
> > >        rcu_process_callbacks+0x100a/0x1ac0 kernel/rcu/tree.c:2697
> > >      ...
> > >
> > > They were caused by the fib_nh_common percpu member
> > > 'nhc_pcpu_rth_output'
> > > overwritten by another percpu variable 'dev->tstats' access overflow
> > > in tipc udp media xmit path when counting packets on a non tunnel device.
> > >
> > > The fix is to make udp tunnel work with no tunnel device by allowing
> > > not to count packets on the tstats when the tunnel dev is NULL in
> > > Patches 1/3 and 2/3, then pass a NULL tunnel dev in tipc_udp_tunnel() in
> Patch 3/3.
> > >
> > > Xin Long (3):
> > >   ip_tunnel: allow not to count pkts on tstats by setting skb's dev to
> > >     NULL
> > >   ip6_tunnel: allow not to count pkts on tstats by passing dev as NULL
> > >   tipc: pass tunnel dev as NULL to udp_tunnel(6)_xmit_skb
> > >
> > >  include/net/ip6_tunnel.h  | 9 ++++++---  net/ipv4/ip_tunnel_core.c
> > > | 9
> > > ++++++---
> > >  net/tipc/udp_media.c      | 8 +++-----
> > >  3 files changed, 15 insertions(+), 11 deletions(-)
> > >
> > > --
> > > 2.1.0
> >

Re: [tipc-discussion] [PATCH net] tipc: add dst_cache support for udp media

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

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

> -----Original Message-----
> From: net...@vg... <net...@vg...> On
> Behalf Of Xin Long
> Sent: 20-Jun-19 07:04
> To: network dev <ne...@vg...>
> Cc: da...@da...; Jon Maloy <jon...@er...>; Ying Xue
> <yin...@wi...>; tip...@li...; Paolo
> Abeni <pa...@re...>
> Subject: [PATCH net] tipc: add dst_cache support for udp media
> 
> As other udp/ip tunnels do, tipc udp media should also have a lockless
> dst_cache supported on its tx path.
> 
> Here we add dst_cache into udp_replicast to support dst cache for both
> rmcast and rcast, and rmcast uses ub->rcast and each rcast uses its own node
> in ub->rcast.list.
> 
> Signed-off-by: Xin Long <luc...@gm...>
> ---
>  net/tipc/udp_media.c | 72 ++++++++++++++++++++++++++++++++++-------
> -----------
>  1 file changed, 47 insertions(+), 25 deletions(-)
> 
> diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c index
> 1405ccc..b8962df 100644
> --- a/net/tipc/udp_media.c
> +++ b/net/tipc/udp_media.c
> @@ -76,6 +76,7 @@ struct udp_media_addr {
>  /* struct udp_replicast - container for UDP remote addresses */  struct
> udp_replicast {
>  	struct udp_media_addr addr;
> +	struct dst_cache dst_cache;
>  	struct rcu_head rcu;
>  	struct list_head list;
>  };
> @@ -158,22 +159,27 @@ static int tipc_udp_addr2msg(char *msg, struct
> tipc_media_addr *a)
>  /* tipc_send_msg - enqueue a send request */  static int tipc_udp_xmit(struct
> net *net, struct sk_buff *skb,
>  			 struct udp_bearer *ub, struct udp_media_addr *src,
> -			 struct udp_media_addr *dst)
> +			 struct udp_media_addr *dst, struct dst_cache *cache)
>  {
> +	struct dst_entry *ndst = dst_cache_get(cache);
>  	int ttl, err = 0;
> -	struct rtable *rt;
> 
>  	if (dst->proto == htons(ETH_P_IP)) {
> -		struct flowi4 fl = {
> -			.daddr = dst->ipv4.s_addr,
> -			.saddr = src->ipv4.s_addr,
> -			.flowi4_mark = skb->mark,
> -			.flowi4_proto = IPPROTO_UDP
> -		};
> -		rt = ip_route_output_key(net, &fl);
> -		if (IS_ERR(rt)) {
> -			err = PTR_ERR(rt);
> -			goto tx_error;
> +		struct rtable *rt = (struct rtable *)ndst;
> +
> +		if (!rt) {
> +			struct flowi4 fl = {
> +				.daddr = dst->ipv4.s_addr,
> +				.saddr = src->ipv4.s_addr,
> +				.flowi4_mark = skb->mark,
> +				.flowi4_proto = IPPROTO_UDP
> +			};
> +			rt = ip_route_output_key(net, &fl);
> +			if (IS_ERR(rt)) {
> +				err = PTR_ERR(rt);
> +				goto tx_error;
> +			}
> +			dst_cache_set_ip4(cache, &rt->dst, fl.saddr);
>  		}
> 
>  		ttl = ip4_dst_hoplimit(&rt->dst);
> @@ -182,17 +188,19 @@ static int tipc_udp_xmit(struct net *net, struct
> sk_buff *skb,
>  				    dst->port, false, true);
>  #if IS_ENABLED(CONFIG_IPV6)
>  	} else {
> -		struct dst_entry *ndst;
> -		struct flowi6 fl6 = {
> -			.flowi6_oif = ub->ifindex,
> -			.daddr = dst->ipv6,
> -			.saddr = src->ipv6,
> -			.flowi6_proto = IPPROTO_UDP
> -		};
> -		err = ipv6_stub->ipv6_dst_lookup(net, ub->ubsock->sk, &ndst,
> -						 &fl6);
> -		if (err)
> -			goto tx_error;
> +		if (!ndst) {
> +			struct flowi6 fl6 = {
> +				.flowi6_oif = ub->ifindex,
> +				.daddr = dst->ipv6,
> +				.saddr = src->ipv6,
> +				.flowi6_proto = IPPROTO_UDP
> +			};
> +			err = ipv6_stub->ipv6_dst_lookup(net, ub->ubsock->sk,
> +							 &ndst, &fl6);
> +			if (err)
> +				goto tx_error;
> +			dst_cache_set_ip6(cache, ndst, &fl6.saddr);
> +		}
>  		ttl = ip6_dst_hoplimit(ndst);
>  		err = udp_tunnel6_xmit_skb(ndst, ub->ubsock->sk, skb, NULL,
>  					   &src->ipv6, &dst->ipv6, 0, ttl, 0, @@ -230,7
> +238,8 @@ static int tipc_udp_send_msg(struct net *net, struct sk_buff *skb,
>  	}
> 
>  	if (addr->broadcast != TIPC_REPLICAST_SUPPORT)
> -		return tipc_udp_xmit(net, skb, ub, src, dst);
> +		return tipc_udp_xmit(net, skb, ub, src, dst,
> +				     &ub->rcast.dst_cache);
> 
>  	/* Replicast, send an skb to each configured IP address */
>  	list_for_each_entry_rcu(rcast, &ub->rcast.list, list) { @@ -242,7 +251,8
> @@ static int tipc_udp_send_msg(struct net *net, struct sk_buff *skb,
>  			goto out;
>  		}
> 
> -		err = tipc_udp_xmit(net, _skb, ub, src, &rcast->addr);
> +		err = tipc_udp_xmit(net, _skb, ub, src, &rcast->addr,
> +				    &rcast->dst_cache);
>  		if (err)
>  			goto out;
>  	}
> @@ -286,6 +296,11 @@ static int tipc_udp_rcast_add(struct tipc_bearer *b,
>  	if (!rcast)
>  		return -ENOMEM;
> 
> +	if (dst_cache_init(&rcast->dst_cache, GFP_ATOMIC)) {
> +		kfree(rcast);
> +		return -ENOMEM;
> +	}
> +
>  	memcpy(&rcast->addr, addr, sizeof(struct udp_media_addr));
> 
>  	if (ntohs(addr->proto) == ETH_P_IP)
> @@ -742,6 +757,10 @@ static int tipc_udp_enable(struct net *net, struct
> tipc_bearer *b,
>  	tuncfg.encap_destroy = NULL;
>  	setup_udp_tunnel_sock(net, ub->ubsock, &tuncfg);
> 
> +	err = dst_cache_init(&ub->rcast.dst_cache, GFP_ATOMIC);
> +	if (err)
> +		goto err;
> +
>  	/**
>  	 * The bcast media address port is used for all peers and the ip
>  	 * is used if it's a multicast address.
> @@ -756,6 +775,7 @@ static int tipc_udp_enable(struct net *net, struct
> tipc_bearer *b,
> 
>  	return 0;
>  err:
> +	dst_cache_destroy(&ub->rcast.dst_cache);
>  	if (ub->ubsock)
>  		udp_tunnel_sock_release(ub->ubsock);
>  	kfree(ub);
> @@ -769,10 +789,12 @@ static void cleanup_bearer(struct work_struct
> *work)
>  	struct udp_replicast *rcast, *tmp;
> 
>  	list_for_each_entry_safe(rcast, tmp, &ub->rcast.list, list) {
> +		dst_cache_destroy(&rcast->dst_cache);
>  		list_del_rcu(&rcast->list);
>  		kfree_rcu(rcast, rcu);
>  	}
> 
> +	dst_cache_destroy(&ub->rcast.dst_cache);
>  	if (ub->ubsock)
>  		udp_tunnel_sock_release(ub->ubsock);
>  	synchronize_net();
> --
> 2.1.0