Netdev List
 help / color / mirror / Atom feed
* [PATCH] Revert "openvswitch: Fix template leak in error cases."
From: Flavio Leitner @ 2018-09-28 17:55 UTC (permalink / raw)
  To: netfilter-devel, netdev, dev
  Cc: Pravin B Shelar, Joe Stringer, Flavio Leitner

This reverts commit 90c7afc96cbbd77f44094b5b651261968e97de67.

When the commit was merged, the code used nf_ct_put() to free
the entry, but later on commit 76644232e612 ("openvswitch: Free
tmpl with tmpl_free.") replaced that with nf_ct_tmpl_free which
is a more appropriate. Now the original problem is removed.

Then 44d6e2f27328 ("net: Replace NF_CT_ASSERT() with WARN_ON().")
replaced a debug assert with a WARN_ON() which is trigged now.

Signed-off-by: Flavio Leitner <fbl@redhat.com>
---
 net/openvswitch/conntrack.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 86a75105af1a..0aeb34c6389d 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -1624,10 +1624,6 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr,
 		OVS_NLERR(log, "Failed to allocate conntrack template");
 		return -ENOMEM;
 	}
-
-	__set_bit(IPS_CONFIRMED_BIT, &ct_info.ct->status);
-	nf_conntrack_get(&ct_info.ct->ct_general);
-
 	if (helper) {
 		err = ovs_ct_add_helper(&ct_info, helper, key, log);
 		if (err)
@@ -1639,6 +1635,8 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr,
 	if (err)
 		goto err_free_ct;
 
+	__set_bit(IPS_CONFIRMED_BIT, &ct_info.ct->status);
+	nf_conntrack_get(&ct_info.ct->ct_general);
 	return 0;
 err_free_ct:
 	__ovs_ct_free_action(&ct_info);
-- 
2.14.4

^ permalink raw reply related

* Re: [PATCH net] vxlan: use nla_put_flag for ttl inherit
From: David Ahern @ 2018-09-28 17:56 UTC (permalink / raw)
  To: Hangbin Liu, David Miller; +Cc: Phil Sutter, netdev, Stephen Hemminger
In-Reply-To: <20180928123858.GF24677@leo.usersys.redhat.com>

On 9/28/18 6:38 AM, Hangbin Liu wrote:
> On Fri, Sep 28, 2018 at 12:37:00PM +0200, Phil Sutter wrote:
>> On Fri, Sep 28, 2018 at 09:08:26AM +0800, Hangbin Liu wrote:
>>> Phil pointed out that there is a mismatch between vxlan and geneve ttl inherit.
>>> We should define it as a flag and use nla_put_flag to export this opiton.
>>
>> s/opiton/option/
>>
>> Apart from that, LGTM!
>>
>> Thanks, Phil
> 
> Opps, sorry...
> 
> Hi David,
> 
> Should I re-send a patch or will you help fix it directly?
> 
> Thanks
> Hangbin
> 

you have this targeted at net; is it a bug in current iproute2 or an
update to a new feature in -next?

^ permalink raw reply

* Re: [Patch net-next v3] net_sched: change tcf_del_walker() to take idrinfo->lock
From: Cong Wang @ 2018-09-28 17:56 UTC (permalink / raw)
  To: Ido Schimmel
  Cc: Linux Kernel Network Developers, Jiri Pirko, Jamal Hadi Salim,
	Vlad Buslov
In-Reply-To: <20180928145900.GA17640@splinter>

On Fri, Sep 28, 2018 at 7:59 AM Ido Schimmel <idosch@idosch.org> wrote:
>
> On Wed, Sep 19, 2018 at 04:37:29PM -0700, Cong Wang wrote:
> > From: Vlad Buslov <vladbu@mellanox.com>
> >
> > From: Vlad Buslov <vladbu@mellanox.com>
> >
> > Action API was changed to work with actions and action_idr in concurrency
> > safe manner, however tcf_del_walker() still uses actions without taking a
> > reference or idrinfo->lock first, and deletes them directly, disregarding
> > possible concurrent delete.
> >
> > Change tcf_del_walker() to take idrinfo->lock while iterating over actions
> > and use new tcf_idr_release_unsafe() to release them while holding the
> > lock.
> >
> > And the blocking function fl_hw_destroy_tmplt() could be called when we
> > put a filter chain, so defer it to a work queue.
>
> I'm getting a use-after-free when running tc_chains.sh selftest and I
> believe it's caused by this patch.
>
> To reproduce:
> # cd tools/testing/selftests/net/forwarding
> # export TESTS="template_filter_fits"; ./tc_chains.sh veth0 veth1
>
> __tcf_chain_put()
>         tc_chain_tmplt_del()
>                 fl_tmplt_destroy()
>                         tcf_queue_work(&tmplt->rwork, fl_tmplt_destroy_work)
>         tcf_chain_destroy()
>                 kfree(chain)
>
> Some time later fl_tmplt_destroy_work() starts executing and
> dereferencing 'chain'.

Oops, forgot to hold the chain... I will test this:

diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 92dd5071a708..cbb68d5515d6 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -1444,6 +1444,7 @@ static void fl_tmplt_destroy_work(struct
work_struct *work)
                                                 struct fl_flow_tmplt, rwork);

        fl_hw_destroy_tmplt(tmplt->chain, tmplt);
+       tcf_chain_put(tmplt->chain);
        kfree(tmplt);
 }

@@ -1451,6 +1452,7 @@ static void fl_tmplt_destroy(void *tmplt_priv)
 {
        struct fl_flow_tmplt *tmplt = tmplt_priv;

+       tcf_chain_hold(tmplt->chain);
        tcf_queue_work(&tmplt->rwork, fl_tmplt_destroy_work);
 }

^ permalink raw reply related

* Re: [PATCH net] vxlan: use nla_put_flag for ttl inherit
From: David Ahern @ 2018-09-28 17:57 UTC (permalink / raw)
  To: Hangbin Liu, netdev; +Cc: David Miller, Stephen Hemminger, Phil Sutter
In-Reply-To: <1538096906-20866-1-git-send-email-liuhangbin@gmail.com>

On 9/27/18 7:08 PM, Hangbin Liu wrote:
> Phil pointed out that there is a mismatch between vxlan and geneve ttl inherit.
> We should define it as a flag and use nla_put_flag to export this opiton.
> 
> Fixes: 8fd780698745b ("vxlan: fill ttl inherit info")

Wrong Fixes tag:

kenny:mgmt:iproute2-next.git$ git show 8fd780698745b
fatal: ambiguous argument '8fd780698745b': unknown revision or path not
in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

> Reported-by: Phil Sutter <phil@nwl.cc>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  drivers/net/vxlan.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 2b8da2b..479dda4 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -3539,7 +3539,7 @@ static size_t vxlan_get_size(const struct net_device *dev)
>  		nla_total_size(sizeof(__u32)) +	/* IFLA_VXLAN_LINK */
>  		nla_total_size(sizeof(struct in6_addr)) + /* IFLA_VXLAN_LOCAL{6} */
>  		nla_total_size(sizeof(__u8)) +	/* IFLA_VXLAN_TTL */
> -		nla_total_size(sizeof(__u8)) +	/* IFLA_VXLAN_TTL_INHERIT */
> +		nla_total_size(0) +		/* IFLA_VXLAN_TTL_INHERIT */
>  		nla_total_size(sizeof(__u8)) +	/* IFLA_VXLAN_TOS */
>  		nla_total_size(sizeof(__be32)) + /* IFLA_VXLAN_LABEL */
>  		nla_total_size(sizeof(__u8)) +	/* IFLA_VXLAN_LEARNING */
> @@ -3604,8 +3604,6 @@ static int vxlan_fill_info(struct sk_buff *skb, const struct net_device *dev)
>  	}
>  
>  	if (nla_put_u8(skb, IFLA_VXLAN_TTL, vxlan->cfg.ttl) ||
> -	    nla_put_u8(skb, IFLA_VXLAN_TTL_INHERIT,
> -		       !!(vxlan->cfg.flags & VXLAN_F_TTL_INHERIT)) ||
>  	    nla_put_u8(skb, IFLA_VXLAN_TOS, vxlan->cfg.tos) ||
>  	    nla_put_be32(skb, IFLA_VXLAN_LABEL, vxlan->cfg.label) ||
>  	    nla_put_u8(skb, IFLA_VXLAN_LEARNING,
> @@ -3650,6 +3648,10 @@ static int vxlan_fill_info(struct sk_buff *skb, const struct net_device *dev)
>  	    nla_put_flag(skb, IFLA_VXLAN_REMCSUM_NOPARTIAL))
>  		goto nla_put_failure;
>  
> +	if (vxlan->cfg.flags & VXLAN_F_TTL_INHERIT &&
> +	    nla_put_flag(skb, IFLA_VXLAN_TTL_INHERIT))
> +		goto nla_put_failure;
> +
>  	return 0;
>  
>  nla_put_failure:
> 

^ permalink raw reply

* Re: [PATCH net-next] geneve: fix ttl inherit type
From: David Ahern @ 2018-09-28 17:59 UTC (permalink / raw)
  To: Hangbin Liu, netdev; +Cc: David Miller, Stephen Hemminger, Phil Sutter
In-Reply-To: <1538096998-20937-1-git-send-email-liuhangbin@gmail.com>

On 9/27/18 7:09 PM, Hangbin Liu wrote:
> Phil pointed out that there is a mismatch between vxlan and geneve ttl
> inherit. We should define it as a flag and use nla_put_flag to export this
> opiton.
> 
> Fixes: 52d0d404d39dd ("geneve: add ttl inherit support")

same here .. getting an unknown commit id.


> Reported-by: Phil Sutter <phil@nwl.cc>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  drivers/net/geneve.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index 6625fab..09ab2fd 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -1100,7 +1100,7 @@ static const struct nla_policy geneve_policy[IFLA_GENEVE_MAX + 1] = {
>  	[IFLA_GENEVE_UDP_CSUM]		= { .type = NLA_U8 },
>  	[IFLA_GENEVE_UDP_ZERO_CSUM6_TX]	= { .type = NLA_U8 },
>  	[IFLA_GENEVE_UDP_ZERO_CSUM6_RX]	= { .type = NLA_U8 },
> -	[IFLA_GENEVE_TTL_INHERIT]	= { .type = NLA_U8 },
> +	[IFLA_GENEVE_TTL_INHERIT]	= { .type = NLA_FLAG },
>  };
>  
>  static int geneve_validate(struct nlattr *tb[], struct nlattr *data[],
> @@ -1582,7 +1582,7 @@ static size_t geneve_get_size(const struct net_device *dev)
>  		nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_UDP_CSUM */
>  		nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_UDP_ZERO_CSUM6_TX */
>  		nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_UDP_ZERO_CSUM6_RX */
> -		nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_TTL_INHERIT */
> +		nla_total_size(0) + 	/* IFLA_GENEVE_TTL_INHERIT */
>  		0;
>  }
>  
> @@ -1636,7 +1636,7 @@ static int geneve_fill_info(struct sk_buff *skb, const struct net_device *dev)
>  		goto nla_put_failure;
>  #endif
>  
> -	if (nla_put_u8(skb, IFLA_GENEVE_TTL_INHERIT, ttl_inherit))
> +	if (ttl_inherit && nla_put_flag(skb, IFLA_GENEVE_TTL_INHERIT))
>  		goto nla_put_failure;
>  
>  	return 0;
> 

^ permalink raw reply

* Re: [PATCH net-next] net: sched: make function qdisc_free_cb() static
From: David Miller @ 2018-09-28 18:06 UTC (permalink / raw)
  To: weiyongjun1; +Cc: jhs, xiyou.wangcong, jiri, vladbu, netdev, kernel-janitors
In-Reply-To: <1538059676-71316-1-git-send-email-weiyongjun1@huawei.com>

From: Wei Yongjun <weiyongjun1@huawei.com>
Date: Thu, 27 Sep 2018 14:47:56 +0000

> Fixes the following sparse warning:
> 
> net/sched/sch_generic.c:944:6: warning:
>  symbol 'qdisc_free_cb' was not declared. Should it be static?
> 
> Fixes: 3a7d0d07a386 ("net: sched: extend Qdisc with rcu")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>

Applied.

^ permalink raw reply

* Re: [Patch net-next v3] net_sched: change tcf_del_walker() to take idrinfo->lock
From: Ido Schimmel @ 2018-09-28 18:11 UTC (permalink / raw)
  To: Cong Wang
  Cc: Linux Kernel Network Developers, Jiri Pirko, Jamal Hadi Salim,
	Vlad Buslov
In-Reply-To: <CAM_iQpUav4UK6uzpaZejFiLn_eEbbvZQry7G6JgzSkj_Eq0fyQ@mail.gmail.com>

On Fri, Sep 28, 2018 at 10:56:47AM -0700, Cong Wang wrote:
> On Fri, Sep 28, 2018 at 7:59 AM Ido Schimmel <idosch@idosch.org> wrote:
> >
> > On Wed, Sep 19, 2018 at 04:37:29PM -0700, Cong Wang wrote:
> > > From: Vlad Buslov <vladbu@mellanox.com>
> > >
> > > From: Vlad Buslov <vladbu@mellanox.com>
> > >
> > > Action API was changed to work with actions and action_idr in concurrency
> > > safe manner, however tcf_del_walker() still uses actions without taking a
> > > reference or idrinfo->lock first, and deletes them directly, disregarding
> > > possible concurrent delete.
> > >
> > > Change tcf_del_walker() to take idrinfo->lock while iterating over actions
> > > and use new tcf_idr_release_unsafe() to release them while holding the
> > > lock.
> > >
> > > And the blocking function fl_hw_destroy_tmplt() could be called when we
> > > put a filter chain, so defer it to a work queue.
> >
> > I'm getting a use-after-free when running tc_chains.sh selftest and I
> > believe it's caused by this patch.
> >
> > To reproduce:
> > # cd tools/testing/selftests/net/forwarding
> > # export TESTS="template_filter_fits"; ./tc_chains.sh veth0 veth1
> >
> > __tcf_chain_put()
> >         tc_chain_tmplt_del()
> >                 fl_tmplt_destroy()
> >                         tcf_queue_work(&tmplt->rwork, fl_tmplt_destroy_work)
> >         tcf_chain_destroy()
> >                 kfree(chain)
> >
> > Some time later fl_tmplt_destroy_work() starts executing and
> > dereferencing 'chain'.
> 
> Oops, forgot to hold the chain... I will test this:
> 
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index 92dd5071a708..cbb68d5515d6 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -1444,6 +1444,7 @@ static void fl_tmplt_destroy_work(struct
> work_struct *work)
>                                                  struct fl_flow_tmplt, rwork);
> 
>         fl_hw_destroy_tmplt(tmplt->chain, tmplt);
> +       tcf_chain_put(tmplt->chain);
>         kfree(tmplt);
>  }
> 
> @@ -1451,6 +1452,7 @@ static void fl_tmplt_destroy(void *tmplt_priv)
>  {
>         struct fl_flow_tmplt *tmplt = tmplt_priv;
> 
> +       tcf_chain_hold(tmplt->chain);
>         tcf_queue_work(&tmplt->rwork, fl_tmplt_destroy_work);
>  }

I don't think this will work given the reference count already dropped
to 0, which is why the template deletion function was invoked. I didn't
test the patch, but I don't see what would prevent the chain from being
freed.

Thanks for looking into this.

^ permalink raw reply

* Re: [net-next 0/8][pull request] 100GbE Intel Wired LAN Driver Updates 2018-09-27
From: David Miller @ 2018-09-28 18:11 UTC (permalink / raw)
  To: jeffrey.t.kirsher; +Cc: netdev, nhorman, sassmann, jogreene
In-Reply-To: <20180927162201.30900-1-jeffrey.t.kirsher@intel.com>

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Thu, 27 Sep 2018 09:21:53 -0700

> This series contains fixes to the ice driver only.
 ...
> The following are changes since commit 1042caa79e9351b81ed19dc8d2d7fd6ff51a4422:
>   net-ipv4: remove 2 always zero parameters from ipv4_redirect()
> and are available in the git repository at:
>   git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue 100GbE

Pulled, thanks Jeff.

^ permalink raw reply

* Re: [PATCH net 00/11] netpoll: second round of fixes.
From: David Miller @ 2018-09-28 18:14 UTC (permalink / raw)
  To: edumazet
  Cc: netdev, michael.chan, aviad.krawczyk, songliubraving, dougmill,
	yisen.zhuang, mst, jasowang, harish.patil, manish.chopra, netanel,
	linux-net-drivers, tlfalcon
In-Reply-To: <20180927163201.56609-1-edumazet@google.com>

From: Eric Dumazet <edumazet@google.com>
Date: Thu, 27 Sep 2018 09:31:50 -0700

> As diagnosed by Song Liu, ndo_poll_controller() can
> be very dangerous on loaded hosts, since the cpu
> calling ndo_poll_controller() might steal all NAPI
> contexts (for all RX/TX queues of the NIC).
> 
> This capture, showing one ksoftirqd eating all cycles
> can last for unlimited amount of time, since one
> cpu is generally not able to drain all the queues under load.
> 
> It seems that all networking drivers that do use NAPI
> for their TX completions, should not provide a ndo_poll_controller() :
> 
> Most NAPI drivers have netpoll support already handled
> in core networking stack, since netpoll_poll_dev()
> uses poll_napi(dev) to iterate through registered
> NAPI contexts for a device.
> 
> First patch is a fix in poll_one_napi().
> 
> Then following patches take care of ten drivers.

Series applied, thanks Eric.

^ permalink raw reply

* [PATCH net v2] net/ncsi: Extend NC-SI Netlink interface to allow user space to send NC-SI command
From: Justin.Lee1 @ 2018-09-28 18:15 UTC (permalink / raw)
  To: joel, sam; +Cc: linux-aspeed, netdev, openbmc, amithash, christian, vijaykhemka

The new command (NCSI_CMD_SEND_CMD) is added to allow user space application 
to send NC-SI command to the network card.
Also, add a new attribute (NCSI_ATTR_DATA) for transferring request and response.

The work flow is as below. 

Request:
User space application -> Netlink interface (msg)
                                              -> new Netlink handler - ncsi_send_cmd_nl()
                                              -> ncsi_xmit_cmd()
Response:
Response received - ncsi_rcv_rsp() -> internal response handler - ncsi_rsp_handler_xxx()
                                                                        -> ncsi_rsp_handler_netlink()
                                                                        -> ncsi_send_netlink_rsp ()
                                                                        -> Netlink interface (msg)
                                                                        -> user space application
Command timeout - ncsi_request_timeout() -> ncsi_send_netlink_timeout ()
                                                                                            -> Netlink interface (msg with zero data length)
                                                                                            -> user space application
Error:
Error detected -> ncsi_send_netlink_err () -> Netlink interface (err msg)
                                                                                       -> user space application


Signed-off-by: Justin Lee <justin.lee1@dell.com>

---
 include/uapi/linux/ncsi.h |   3 +
 net/ncsi/internal.h       |  12 ++-
 net/ncsi/ncsi-cmd.c       |  47 ++++++++++-
 net/ncsi/ncsi-manage.c    |  22 +++++
 net/ncsi/ncsi-netlink.c   | 205 ++++++++++++++++++++++++++++++++++++++++++++++
 net/ncsi/ncsi-netlink.h   |  12 +++
 net/ncsi/ncsi-rsp.c       |  71 ++++++++++++++--
 7 files changed, 363 insertions(+), 9 deletions(-)

diff --git a/include/uapi/linux/ncsi.h b/include/uapi/linux/ncsi.h
index 4c292ec..4992bfc 100644
--- a/include/uapi/linux/ncsi.h
+++ b/include/uapi/linux/ncsi.h
@@ -30,6 +30,7 @@ enum ncsi_nl_commands {
 	NCSI_CMD_PKG_INFO,
 	NCSI_CMD_SET_INTERFACE,
 	NCSI_CMD_CLEAR_INTERFACE,
+	NCSI_CMD_SEND_CMD,
 
 	__NCSI_CMD_AFTER_LAST,
 	NCSI_CMD_MAX = __NCSI_CMD_AFTER_LAST - 1
@@ -43,6 +44,7 @@ enum ncsi_nl_commands {
  * @NCSI_ATTR_PACKAGE_LIST: nested array of NCSI_PKG_ATTR attributes
  * @NCSI_ATTR_PACKAGE_ID: package ID
  * @NCSI_ATTR_CHANNEL_ID: channel ID
+ * @NCSI_ATTR_DATA: command payload
  * @NCSI_ATTR_MAX: highest attribute number
  */
 enum ncsi_nl_attrs {
@@ -51,6 +53,7 @@ enum ncsi_nl_attrs {
 	NCSI_ATTR_PACKAGE_LIST,
 	NCSI_ATTR_PACKAGE_ID,
 	NCSI_ATTR_CHANNEL_ID,
+	NCSI_ATTR_DATA,
 
 	__NCSI_ATTR_AFTER_LAST,
 	NCSI_ATTR_MAX = __NCSI_ATTR_AFTER_LAST - 1
diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 8055e39..1a3ef9e 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -171,6 +171,8 @@ struct ncsi_package;
 #define NCSI_RESERVED_CHANNEL	0x1f
 #define NCSI_CHANNEL_INDEX(c)	((c) & ((1 << NCSI_PACKAGE_SHIFT) - 1))
 #define NCSI_TO_CHANNEL(p, c)	(((p) << NCSI_PACKAGE_SHIFT) | (c))
+#define NCSI_MAX_PACKAGE	8
+#define NCSI_MAX_CHANNEL	32
 
 struct ncsi_channel {
 	unsigned char               id;
@@ -215,12 +217,17 @@ struct ncsi_request {
 	unsigned char        id;      /* Request ID - 0 to 255           */
 	bool                 used;    /* Request that has been assigned  */
 	unsigned int         flags;   /* NCSI request property           */
-#define NCSI_REQ_FLAG_EVENT_DRIVEN	1
+#define NCSI_REQ_FLAG_EVENT_DRIVEN		1
+#define NCSI_REQ_FLAG_NETLINK_DRIVEN	2
 	struct ncsi_dev_priv *ndp;    /* Associated NCSI device          */
 	struct sk_buff       *cmd;    /* Associated NCSI command packet  */
 	struct sk_buff       *rsp;    /* Associated NCSI response packet */
 	struct timer_list    timer;   /* Timer on waiting for response   */
 	bool                 enabled; /* Time has been enabled or not    */
+
+	u32                  snd_seq;     /* netlink sending sequence number */
+	u32                  snd_portid;  /* netlink portid of sender        */
+	struct nlmsghdr      nlhdr;       /* netlink message header          */
 };
 
 enum {
@@ -305,6 +312,9 @@ struct ncsi_cmd_arg {
 		unsigned short words[8];
 		unsigned int   dwords[4];
 	};
+
+	unsigned char        *data;       /* Netlink data                  */
+	struct genl_info     *info;       /* Netlink information           */
 };
 
 extern struct list_head ncsi_dev_list;
diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
index 7567ca63..43b544c 100644
--- a/net/ncsi/ncsi-cmd.c
+++ b/net/ncsi/ncsi-cmd.c
@@ -17,6 +17,7 @@
 #include <net/ncsi.h>
 #include <net/net_namespace.h>
 #include <net/sock.h>
+#include <net/genetlink.h>
 
 #include "internal.h"
 #include "ncsi-pkt.h"
@@ -211,6 +212,39 @@ static int ncsi_cmd_handler_snfc(struct sk_buff *skb,
 	return 0;
 }
 
+static int ncsi_cmd_handler_oem(struct sk_buff *skb,
+				struct ncsi_cmd_arg *nca)
+{
+	struct ncsi_cmd_pkt *cmd;
+	unsigned char *dest, *source;
+	unsigned short len;
+
+	/* struct ncsi_cmd_pkt = minimum length
+	 *                       - frame checksum
+	 *                       - Ethernet header
+	 *                     = 64 - 4 - 14 = 46
+	 * minimum payload = 46 - ncsi header - ncsi checksum
+	 *                 = 46 - 16 - 4 = 26
+	 */
+	len = nca->payload;
+
+	/* minimum payload length is 26 bytes to meet minimum packet
+	 * length 64
+	 */
+	if (len < 26)
+		cmd = skb_put_zero(skb, sizeof(*cmd));
+	else
+		cmd = skb_put_zero(skb, len + sizeof(struct ncsi_pkt_hdr) + 4);
+
+	dest = (unsigned char *)cmd + sizeof(struct ncsi_pkt_hdr);
+	source = (unsigned char *)nca->data + sizeof(struct ncsi_pkt_hdr);
+	memcpy(dest, source, len);
+
+	ncsi_cmd_build_header(&cmd->cmd.common, nca);
+
+	return 0;
+}
+
 static struct ncsi_cmd_handler {
 	unsigned char type;
 	int           payload;
@@ -244,7 +278,7 @@ static struct ncsi_cmd_handler {
 	{ NCSI_PKT_CMD_GNS,    0, ncsi_cmd_handler_default },
 	{ NCSI_PKT_CMD_GNPTS,  0, ncsi_cmd_handler_default },
 	{ NCSI_PKT_CMD_GPS,    0, ncsi_cmd_handler_default },
-	{ NCSI_PKT_CMD_OEM,    0, NULL                     },
+	{ NCSI_PKT_CMD_OEM,    -1, ncsi_cmd_handler_oem     },
 	{ NCSI_PKT_CMD_PLDM,   0, NULL                     },
 	{ NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default }
 };
@@ -317,11 +351,20 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
 	}
 
 	/* Get packet payload length and allocate the request */
-	nca->payload = nch->payload;
+	if (nch->payload >= 0)
+		nca->payload = nch->payload;
+
 	nr = ncsi_alloc_command(nca);
 	if (!nr)
 		return -ENOMEM;
 
+	/* track netlink information */
+	if (nca->req_flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) {
+		nr->snd_seq = nca->info->snd_seq;
+		nr->snd_portid = nca->info->snd_portid;
+		nr->nlhdr = *nca->info->nlhdr;
+	}
+
 	/* Prepare the packet */
 	nca->id = nr->id;
 	ret = nch->handler(nr->cmd, nca);
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 0912847..29f33a1 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -19,6 +19,7 @@
 #include <net/addrconf.h>
 #include <net/ipv6.h>
 #include <net/if_inet6.h>
+#include <net/genetlink.h>
 
 #include "internal.h"
 #include "ncsi-pkt.h"
@@ -406,8 +407,13 @@ static void ncsi_request_timeout(struct timer_list *t)
 {
 	struct ncsi_request *nr = from_timer(nr, t, timer);
 	struct ncsi_dev_priv *ndp = nr->ndp;
+	struct ncsi_package *np;
+	struct ncsi_channel *nc;
+	struct ncsi_cmd_pkt *cmd;
 	unsigned long flags;
 
+	netdev_dbg(ndp->ndev.dev, "NCSI: %s\n", __func__);
+
 	/* If the request already had associated response,
 	 * let the response handler to release it.
 	 */
@@ -415,10 +421,26 @@ static void ncsi_request_timeout(struct timer_list *t)
 	nr->enabled = false;
 	if (nr->rsp || !nr->cmd) {
 		spin_unlock_irqrestore(&ndp->lock, flags);
+
+		netdev_dbg(ndp->ndev.dev,
+			   "NCSI: %s - early return\n", __func__);
+
 		return;
 	}
 	spin_unlock_irqrestore(&ndp->lock, flags);
 
+	if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) {
+		if (nr->cmd) {
+			/* Find the package */
+			cmd = (struct ncsi_cmd_pkt *)
+			      skb_network_header(nr->cmd);
+			ncsi_find_package_and_channel(ndp,
+						      cmd->cmd.common.channel,
+						      &np, &nc);
+			ncsi_send_netlink_timeout(nr, np, nc);
+		}
+	}
+
 	/* Release the request */
 	ncsi_free_request(nr);
 }
diff --git a/net/ncsi/ncsi-netlink.c b/net/ncsi/ncsi-netlink.c
index 45f33d6..ce57675 100644
--- a/net/ncsi/ncsi-netlink.c
+++ b/net/ncsi/ncsi-netlink.c
@@ -20,6 +20,7 @@
 #include <uapi/linux/ncsi.h>
 
 #include "internal.h"
+#include "ncsi-pkt.h"
 #include "ncsi-netlink.h"
 
 static struct genl_family ncsi_genl_family;
@@ -29,6 +30,7 @@ static const struct nla_policy ncsi_genl_policy[NCSI_ATTR_MAX + 1] = {
 	[NCSI_ATTR_PACKAGE_LIST] =	{ .type = NLA_NESTED },
 	[NCSI_ATTR_PACKAGE_ID] =	{ .type = NLA_U32 },
 	[NCSI_ATTR_CHANNEL_ID] =	{ .type = NLA_U32 },
+	[NCSI_ATTR_DATA] =		{ .type = NLA_BINARY, .len = 2048 },
 };
 
 static struct ncsi_dev_priv *ndp_from_ifindex(struct net *net, u32 ifindex)
@@ -366,6 +368,203 @@ static int ncsi_clear_interface_nl(struct sk_buff *msg, struct genl_info *info)
 	return 0;
 }
 
+static int ncsi_send_cmd_nl(struct sk_buff *msg, struct genl_info *info)
+{
+	struct ncsi_dev_priv *ndp;
+
+	struct ncsi_cmd_arg nca;
+	struct ncsi_pkt_hdr *hdr;
+
+	u32 package_id, channel_id;
+	unsigned char *data;
+	void *head;
+	int len, ret;
+
+	if (!info || !info->attrs) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (!info->attrs[NCSI_ATTR_IFINDEX]) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (!info->attrs[NCSI_ATTR_PACKAGE_ID]) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (!info->attrs[NCSI_ATTR_CHANNEL_ID]) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ndp = ndp_from_ifindex(get_net(sock_net(msg->sk)),
+			       nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));
+	if (!ndp) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	package_id = nla_get_u32(info->attrs[NCSI_ATTR_PACKAGE_ID]);
+	channel_id = nla_get_u32(info->attrs[NCSI_ATTR_CHANNEL_ID]);
+
+	if (package_id >= NCSI_MAX_PACKAGE || channel_id >= NCSI_MAX_CHANNEL) {
+		ret = -ERANGE;
+		goto out_netlink;
+	}
+
+	len = nla_len(info->attrs[NCSI_ATTR_DATA]);
+	if (len < sizeof(struct ncsi_pkt_hdr)) {
+		netdev_info(ndp->ndev.dev, "NCSI: no OEM command to send %u\n",
+			    package_id);
+		ret = -EINVAL;
+		goto out_netlink;
+	} else {
+		head = nla_data(info->attrs[NCSI_ATTR_DATA]);
+		data = (unsigned char *)head;
+	}
+
+	hdr = (struct ncsi_pkt_hdr *)data;
+
+	nca.ndp = ndp;
+	nca.package = (unsigned char)package_id;
+	nca.channel = (unsigned char)channel_id;
+	nca.type = hdr->type;
+	nca.req_flags = NCSI_REQ_FLAG_NETLINK_DRIVEN;
+	nca.info = info;
+	nca.payload = ntohs(hdr->length);
+	nca.data = data;
+
+	ret = ncsi_xmit_cmd(&nca);
+out_netlink:
+	if (ret != 0) {
+		netdev_err(ndp->ndev.dev,
+			   "Error %d sending OEM command\n", ret);
+		ncsi_send_netlink_err(ndp->ndev.dev,
+				      info->snd_seq,
+				      info->snd_portid,
+				      info->nlhdr,
+				      ret);
+	}
+out:
+	return ret;
+}
+
+int ncsi_send_netlink_rsp(struct ncsi_request *nr,
+			  struct ncsi_package *np,
+			  struct ncsi_channel *nc)
+{
+	struct sk_buff *skb;
+	struct net *net;
+	void *hdr;
+	int rc;
+
+	netdev_dbg(nr->ndp->ndev.dev, "NCSI: %s\n", __func__);
+
+	net = dev_net(nr->rsp->dev);
+
+	skb = genlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
+	if (!skb)
+		return -ENOMEM;
+
+	hdr = genlmsg_put(skb, nr->snd_portid, nr->snd_seq,
+			  &ncsi_genl_family, 0, NCSI_CMD_SEND_CMD);
+	if (!hdr) {
+		kfree_skb(skb);
+		return -EMSGSIZE;
+	}
+
+	nla_put_u32(skb, NCSI_ATTR_IFINDEX, nr->rsp->dev->ifindex);
+	if (np)
+		nla_put_u32(skb, NCSI_ATTR_PACKAGE_ID, np->id);
+	if (nc)
+		nla_put_u32(skb, NCSI_ATTR_CHANNEL_ID, nc->id);
+	else
+		nla_put_u32(skb, NCSI_ATTR_CHANNEL_ID, NCSI_RESERVED_CHANNEL);
+
+	rc = nla_put(skb, NCSI_ATTR_DATA, nr->rsp->len, (void *)nr->rsp->data);
+	if (rc)
+		goto err;
+
+	genlmsg_end(skb, hdr);
+	return genlmsg_unicast(net, skb, nr->snd_portid);
+
+err:
+	kfree_skb(skb);
+	return rc;
+}
+
+int ncsi_send_netlink_timeout(struct ncsi_request *nr,
+			      struct ncsi_package *np,
+			      struct ncsi_channel *nc)
+{
+	struct sk_buff *skb;
+	struct net *net;
+	void *hdr;
+
+	netdev_dbg(nr->ndp->ndev.dev, "NCSI: %s\n", __func__);
+
+	skb = genlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
+	if (!skb)
+		return -ENOMEM;
+
+	hdr = genlmsg_put(skb, nr->snd_portid, nr->snd_seq,
+			  &ncsi_genl_family, 0, NCSI_CMD_SEND_CMD);
+	if (!hdr) {
+		kfree_skb(skb);
+		return -EMSGSIZE;
+	}
+
+	net = dev_net(nr->cmd->dev);
+
+	nla_put_u32(skb, NCSI_ATTR_IFINDEX, nr->cmd->dev->ifindex);
+
+	if (np)
+		nla_put_u32(skb, NCSI_ATTR_PACKAGE_ID, np->id);
+	else
+		nla_put_u32(skb, NCSI_ATTR_PACKAGE_ID,
+			    NCSI_PACKAGE_INDEX((((struct ncsi_pkt_hdr *)
+						 nr->cmd->data)->channel)));
+
+	if (nc)
+		nla_put_u32(skb, NCSI_ATTR_CHANNEL_ID, nc->id);
+	else
+		nla_put_u32(skb, NCSI_ATTR_CHANNEL_ID, NCSI_RESERVED_CHANNEL);
+
+	genlmsg_end(skb, hdr);
+	return genlmsg_unicast(net, skb, nr->snd_portid);
+}
+
+int ncsi_send_netlink_err(struct net_device *dev,
+			  u32 snd_seq,
+			  u32 snd_portid,
+			  struct nlmsghdr *nlhdr,
+			  int err)
+{
+	struct sk_buff *skb;
+	struct nlmsghdr *nlh;
+	struct nlmsgerr *nle;
+	struct net *net;
+
+	skb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
+	if (!skb)
+		return -ENOMEM;
+
+	net = dev_net(dev);
+
+	nlh = nlmsg_put(skb, snd_portid, snd_seq,
+			NLMSG_ERROR, sizeof(*nle), 0);
+	nle = (struct nlmsgerr *)nlmsg_data(nlh);
+	nle->error = err;
+	memcpy(&nle->msg, nlhdr, sizeof(*nlh));
+
+	nlmsg_end(skb, nlh);
+
+	return nlmsg_unicast(net->genl_sock, skb, snd_portid);
+}
+
 static const struct genl_ops ncsi_ops[] = {
 	{
 		.cmd = NCSI_CMD_PKG_INFO,
@@ -386,6 +585,12 @@ static const struct genl_ops ncsi_ops[] = {
 		.doit = ncsi_clear_interface_nl,
 		.flags = GENL_ADMIN_PERM,
 	},
+	{
+		.cmd = NCSI_CMD_SEND_CMD,
+		.policy = ncsi_genl_policy,
+		.doit = ncsi_send_cmd_nl,
+		.flags = GENL_ADMIN_PERM,
+	},
 };
 
 static struct genl_family ncsi_genl_family __ro_after_init = {
diff --git a/net/ncsi/ncsi-netlink.h b/net/ncsi/ncsi-netlink.h
index 91a5c25..c4a4688 100644
--- a/net/ncsi/ncsi-netlink.h
+++ b/net/ncsi/ncsi-netlink.h
@@ -14,6 +14,18 @@
 
 #include "internal.h"
 
+int ncsi_send_netlink_rsp(struct ncsi_request *nr,
+			  struct ncsi_package *np,
+			  struct ncsi_channel *nc);
+int ncsi_send_netlink_timeout(struct ncsi_request *nr,
+			      struct ncsi_package *np,
+			      struct ncsi_channel *nc);
+int ncsi_send_netlink_err(struct net_device *dev,
+			  u32 snd_seq,
+			  u32 snd_portid,
+			  struct nlmsghdr *nlhdr,
+			  int err);
+
 int ncsi_init_netlink(struct net_device *dev);
 int ncsi_unregister_netlink(struct net_device *dev);
 
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 930c1d3..010970f 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -16,9 +16,11 @@
 #include <net/ncsi.h>
 #include <net/net_namespace.h>
 #include <net/sock.h>
+#include <net/genetlink.h>
 
 #include "internal.h"
 #include "ncsi-pkt.h"
+#include "ncsi-netlink.h"
 
 static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
 				 unsigned short payload)
@@ -32,15 +34,22 @@ static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
 	 * before calling this function.
 	 */
 	h = (struct ncsi_rsp_pkt_hdr *)skb_network_header(nr->rsp);
-	if (h->common.revision != NCSI_PKT_REVISION)
+
+	if (h->common.revision != NCSI_PKT_REVISION) {
+		netdev_dbg(nr->ndp->ndev.dev, "NCSI: unsupported header revision\n");
 		return -EINVAL;
-	if (ntohs(h->common.length) != payload)
+	}
+	if (ntohs(h->common.length) != payload) {
+		netdev_dbg(nr->ndp->ndev.dev, "NCSI: payload length mismatched\n");
 		return -EINVAL;
+	}
 
 	/* Check on code and reason */
 	if (ntohs(h->code) != NCSI_PKT_RSP_C_COMPLETED ||
-	    ntohs(h->reason) != NCSI_PKT_RSP_R_NO_ERROR)
-		return -EINVAL;
+	    ntohs(h->reason) != NCSI_PKT_RSP_R_NO_ERROR) {
+		netdev_dbg(nr->ndp->ndev.dev, "NCSI: non zero response/reason code\n");
+		return -EPERM;
+	}
 
 	/* Validate checksum, which might be zeroes if the
 	 * sender doesn't support checksum according to NCSI
@@ -52,8 +61,11 @@ static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
 
 	checksum = ncsi_calculate_checksum((unsigned char *)h,
 					   sizeof(*h) + payload - 4);
-	if (*pchecksum != htonl(checksum))
+
+	if (*pchecksum != htonl(checksum)) {
+		netdev_dbg(nr->ndp->ndev.dev, "NCSI: checksum mismatched\n");
 		return -EINVAL;
+	}
 
 	return 0;
 }
@@ -900,6 +912,31 @@ static int ncsi_rsp_handler_gpuuid(struct ncsi_request *nr)
 	return 0;
 }
 
+static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
+{
+	return 0;
+}
+
+static int ncsi_rsp_handler_netlink(struct ncsi_request *nr)
+{
+	struct ncsi_rsp_pkt *rsp;
+	struct ncsi_dev_priv *ndp = nr->ndp;
+	struct ncsi_package *np;
+	struct ncsi_channel *nc;
+	int ret;
+
+	/* Find the package */
+	rsp = (struct ncsi_rsp_pkt *)skb_network_header(nr->rsp);
+	ncsi_find_package_and_channel(ndp, rsp->rsp.common.channel,
+				      &np, &nc);
+	if (!np)
+		return -ENODEV;
+
+	ret = ncsi_send_netlink_rsp(nr, np, nc);
+
+	return ret;
+}
+
 static struct ncsi_rsp_handler {
 	unsigned char	type;
 	int             payload;
@@ -932,7 +969,7 @@ static struct ncsi_rsp_handler {
 	{ NCSI_PKT_RSP_GNS,   172, ncsi_rsp_handler_gns     },
 	{ NCSI_PKT_RSP_GNPTS, 172, ncsi_rsp_handler_gnpts   },
 	{ NCSI_PKT_RSP_GPS,     8, ncsi_rsp_handler_gps     },
-	{ NCSI_PKT_RSP_OEM,     0, NULL                     },
+	{ NCSI_PKT_RSP_OEM,    -1, ncsi_rsp_handler_oem     },
 	{ NCSI_PKT_RSP_PLDM,    0, NULL                     },
 	{ NCSI_PKT_RSP_GPUUID, 20, ncsi_rsp_handler_gpuuid  }
 };
@@ -1002,6 +1039,17 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
 		netdev_warn(ndp->ndev.dev,
 			    "NCSI: 'bad' packet ignored for type 0x%x\n",
 			    hdr->type);
+
+		if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) {
+			if (ret == -EPERM)
+				goto out_netlink;
+			else
+				ncsi_send_netlink_err(ndp->ndev.dev,
+						      nr->snd_seq,
+						      nr->snd_portid,
+						      &nr->nlhdr,
+						      ret);
+		}
 		goto out;
 	}
 
@@ -1011,6 +1059,17 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
 		netdev_err(ndp->ndev.dev,
 			   "NCSI: Handler for packet type 0x%x returned %d\n",
 			   hdr->type, ret);
+
+out_netlink:
+	if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) {
+		ret = ncsi_rsp_handler_netlink(nr);
+		if (ret) {
+			netdev_err(ndp->ndev.dev,
+				   "NCSI: Netlink handler for packet type 0x%x returned %d\n",
+				   hdr->type, ret);
+		}
+	}
+
 out:
 	ncsi_free_request(nr);
 	return ret;
-- 
2.9.3

^ permalink raw reply related

* Re: bond: take rcu lock in bond_poll_controller
From: Dave Jones @ 2018-09-28 18:21 UTC (permalink / raw)
  To: Cong Wang; +Cc: Linux Kernel Network Developers
In-Reply-To: <CAM_iQpV67iFZptFfWPXqrFD6GW3JKR78GwocqnA1BeTS3fJaAA@mail.gmail.com>

On Fri, Sep 28, 2018 at 10:31:39AM -0700, Cong Wang wrote:
 > On Fri, Sep 28, 2018 at 10:25 AM Dave Jones <davej@codemonkey.org.uk> wrote:
 > >
 > > On Fri, Sep 28, 2018 at 09:55:52AM -0700, Cong Wang wrote:
 > >  > On Fri, Sep 28, 2018 at 9:18 AM Dave Jones <davej@codemonkey.org.uk> wrote:
 > >  > >
 > >  > > Callers of bond_for_each_slave_rcu are expected to hold the rcu lock,
 > >  > > otherwise a trace like below is shown
 > >  >
 > >  > So why not take rcu read lock in netpoll_send_skb_on_dev() where
 > >  > RCU is also assumed?
 > >
 > > that does seem to solve the backtrace spew I saw too, so if that's
 > > preferable I can respin the patch.
 > 
 > 
 > >From my observations, netpoll_send_skb_on_dev() does not take
 > RCU read lock _and_ it relies on rcu read lock because it calls
 > rcu_dereference_bh().
 > 
 > If my observation is correct, you should catch a RCU warning like
 > this but within netpoll_send_skb_on_dev().
 >
 > >  > As I said, I can't explain why you didn't trigger the RCU warning in
 > >  > netpoll_send_skb_on_dev()...
 > >
 > > netpoll_send_skb_on_dev takes the rcu lock itself.
 > 
 > Could you please point me where exactly is the rcu lock here?
 > 
 > I am too stupid to see it. :)

No, I'm the stupid one. I looked at the tree I had just edited to try your
proposed change. 

Now that I've untangled myself, I'll repost with your suggested change.

	Dave

^ permalink raw reply

* [net-next 5/5] tipc: buffer overflow handling in listener socket
From: Jon Maloy @ 2018-09-28 18:23 UTC (permalink / raw)
  To: davem, netdev
  Cc: gordan.mihaljevic, tung.q.nguyen, hoang.h.le, jon.maloy,
	canh.d.luu, ying.xue, tipc-discussion
In-Reply-To: <1538159002-11800-1-git-send-email-jon.maloy@ericsson.com>

From: Tung Nguyen <tung.q.nguyen@dektech.com.au>

Default socket receive buffer size for a listener socket is 2Mb. For
each arriving empty SYN, the linux kernel allocates a 768 bytes buffer.
This means that a listener socket can serve maximum 2700 simultaneous
empty connection setup requests before it hits a receive buffer
overflow, and much fewer if the SYN is carrying any significant
amount of data.

When this happens the setup request is rejected, and the client
receives an ECONNREFUSED error.

This commit mitigates this problem by letting the client socket try to
retransmit the SYN message multiple times when it sees it rejected with
the code TIPC_ERR_OVERLOAD. Retransmission is done at random intervals
in the range of [100 ms, setup_timeout / 4], as many times as there is
room for within the setup timeout limit.

Signed-off-by: Tung Nguyen <tung.q.nguyen@dektech.com.au>
Acked-by: Ying Xue <ying.xue@windriver.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
 net/tipc/msg.c    | 20 ++++++++++++++++++++
 net/tipc/msg.h    |  1 +
 net/tipc/socket.c | 49 +++++++++++++++++++++++++++++++++++++++++++------
 3 files changed, 64 insertions(+), 6 deletions(-)

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 00fbb5c..f48e585 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -525,6 +525,10 @@ bool tipc_msg_reverse(u32 own_node,  struct sk_buff **skb, int err)
 	if (hlen == SHORT_H_SIZE)
 		hlen = BASIC_H_SIZE;
 
+	/* Don't return data along with SYN+, - sender has a clone */
+	if (msg_is_syn(_hdr) && err == TIPC_ERR_OVERLOAD)
+		dlen = 0;
+
 	/* Allocate new buffer to return */
 	*skb = tipc_buf_acquire(hlen + dlen, GFP_ATOMIC);
 	if (!*skb)
@@ -552,6 +556,22 @@ bool tipc_msg_reverse(u32 own_node,  struct sk_buff **skb, int err)
 	return false;
 }
 
+bool tipc_msg_skb_clone(struct sk_buff_head *msg, struct sk_buff_head *cpy)
+{
+	struct sk_buff *skb, *_skb;
+
+	skb_queue_walk(msg, skb) {
+		_skb = skb_clone(skb, GFP_ATOMIC);
+		if (!_skb) {
+			__skb_queue_purge(cpy);
+			pr_err_ratelimited("Failed to clone buffer chain\n");
+			return false;
+		}
+		__skb_queue_tail(cpy, _skb);
+	}
+	return true;
+}
+
 /**
  * tipc_msg_lookup_dest(): try to find new destination for named message
  * @skb: the buffer containing the message.
diff --git a/net/tipc/msg.h b/net/tipc/msg.h
index cd64d7b..a2879e6 100644
--- a/net/tipc/msg.h
+++ b/net/tipc/msg.h
@@ -980,6 +980,7 @@ bool tipc_msg_pskb_copy(u32 dst, struct sk_buff_head *msg,
 			struct sk_buff_head *cpy);
 void __tipc_skb_queue_sorted(struct sk_buff_head *list, u16 seqno,
 			     struct sk_buff *skb);
+bool tipc_msg_skb_clone(struct sk_buff_head *msg, struct sk_buff_head *cpy);
 
 static inline u16 buf_seqno(struct sk_buff *skb)
 {
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 89d6dc0..595c500 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -47,7 +47,7 @@
 #include "netlink.h"
 #include "group.h"
 
-#define CONN_TIMEOUT_DEFAULT	8000	/* default connect timeout = 8s */
+#define CONN_TIMEOUT_DEFAULT    8000    /* default connect timeout = 8s */
 #define CONN_PROBING_INTV	msecs_to_jiffies(3600000)  /* [ms] => 1 h */
 #define TIPC_FWD_MSG		1
 #define TIPC_MAX_PORT		0xffffffff
@@ -80,7 +80,6 @@ struct sockaddr_pair {
  * @publications: list of publications for port
  * @blocking_link: address of the congested link we are currently sleeping on
  * @pub_count: total # of publications port has made during its lifetime
- * @probing_state:
  * @conn_timeout: the time we can wait for an unresponded setup request
  * @dupl_rcvcnt: number of bytes counted twice, in both backlog and rcv queue
  * @cong_link_cnt: number of congested links
@@ -102,8 +101,8 @@ struct tipc_sock {
 	struct list_head cong_links;
 	struct list_head publications;
 	u32 pub_count;
-	uint conn_timeout;
 	atomic_t dupl_rcvcnt;
+	u16 conn_timeout;
 	bool probe_unacked;
 	u16 cong_link_cnt;
 	u16 snt_unacked;
@@ -507,6 +506,9 @@ static void __tipc_shutdown(struct socket *sock, int error)
 	tipc_wait_for_cond(sock, &timeout, (!tsk->cong_link_cnt &&
 					    !tsk_conn_cong(tsk)));
 
+	/* Remove any pending SYN message */
+	__skb_queue_purge(&sk->sk_write_queue);
+
 	/* Reject all unreceived messages, except on an active connection
 	 * (which disconnects locally & sends a 'FIN+' to peer).
 	 */
@@ -1362,6 +1364,8 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
 	rc = tipc_msg_build(hdr, m, 0, dlen, mtu, &pkts);
 	if (unlikely(rc != dlen))
 		return rc;
+	if (unlikely(syn && !tipc_msg_skb_clone(&pkts, &sk->sk_write_queue)))
+		return -ENOMEM;
 
 	rc = tipc_node_xmit(net, &pkts, dnode, tsk->portid);
 	if (unlikely(rc == -ELINKCONG)) {
@@ -1491,6 +1495,7 @@ static void tipc_sk_finish_conn(struct tipc_sock *tsk, u32 peer_port,
 	tipc_node_add_conn(net, peer_node, tsk->portid, peer_port);
 	tsk->max_pkt = tipc_node_get_mtu(net, peer_node, tsk->portid);
 	tsk->peer_caps = tipc_node_get_capabilities(net, peer_node);
+	__skb_queue_purge(&sk->sk_write_queue);
 	if (tsk->peer_caps & TIPC_BLOCK_FLOWCTL)
 		return;
 
@@ -1977,6 +1982,7 @@ static bool tipc_sk_filter_connect(struct tipc_sock *tsk, struct sk_buff *skb)
 	u32 oport = msg_origport(hdr);
 	u32 onode = msg_orignode(hdr);
 	int err = msg_errcode(hdr);
+	unsigned long delay;
 
 	if (unlikely(msg_mcast(hdr)))
 		return false;
@@ -2001,8 +2007,18 @@ static bool tipc_sk_filter_connect(struct tipc_sock *tsk, struct sk_buff *skb)
 		if (oport != pport || onode != pnode)
 			return false;
 
-		/* Rejected SYN - abort */
-		break;
+		/* Rejected SYN */
+		if (err != TIPC_ERR_OVERLOAD)
+			break;
+
+		/* Prepare for new setup attempt if we have a SYN clone */
+		if (skb_queue_empty(&sk->sk_write_queue))
+			break;
+		get_random_bytes(&delay, 2);
+		delay %= (tsk->conn_timeout / 4);
+		delay = msecs_to_jiffies(delay + 100);
+		sk_reset_timer(sk, &sk->sk_timer, jiffies + delay);
+		return false;
 	case TIPC_OPEN:
 	case TIPC_DISCONNECTING:
 		return false;
@@ -2561,12 +2577,26 @@ static void tipc_sk_check_probing_state(struct sock *sk,
 	sk_reset_timer(sk, &sk->sk_timer, jiffies + CONN_PROBING_INTV);
 }
 
+static void tipc_sk_retry_connect(struct sock *sk, struct sk_buff_head *list)
+{
+	struct tipc_sock *tsk = tipc_sk(sk);
+
+	/* Try again later if dest link is congested */
+	if (tsk->cong_link_cnt) {
+		sk_reset_timer(sk, &sk->sk_timer, msecs_to_jiffies(100));
+		return;
+	}
+	/* Prepare SYN for retransmit */
+	tipc_msg_skb_clone(&sk->sk_write_queue, list);
+}
+
 static void tipc_sk_timeout(struct timer_list *t)
 {
 	struct sock *sk = from_timer(sk, t, sk_timer);
 	struct tipc_sock *tsk = tipc_sk(sk);
 	u32 pnode = tsk_peer_node(tsk);
 	struct sk_buff_head list;
+	int rc = 0;
 
 	skb_queue_head_init(&list);
 	bh_lock_sock(sk);
@@ -2580,12 +2610,19 @@ static void tipc_sk_timeout(struct timer_list *t)
 
 	if (sk->sk_state == TIPC_ESTABLISHED)
 		tipc_sk_check_probing_state(sk, &list);
+	else if (sk->sk_state == TIPC_CONNECTING)
+		tipc_sk_retry_connect(sk, &list);
 
 	bh_unlock_sock(sk);
 
 	if (!skb_queue_empty(&list))
-		tipc_node_xmit(sock_net(sk), &list, pnode, tsk->portid);
+		rc = tipc_node_xmit(sock_net(sk), &list, pnode, tsk->portid);
 
+	/* SYN messages may cause link congestion */
+	if (rc == -ELINKCONG) {
+		tipc_dest_push(&tsk->cong_links, pnode, 0);
+		tsk->cong_link_cnt = 1;
+	}
 	sock_put(sk);
 }
 
-- 
2.1.4

^ permalink raw reply related

* [net-next 1/5] tipc: refactor function tipc_msg_reverse()
From: Jon Maloy @ 2018-09-28 18:23 UTC (permalink / raw)
  To: davem, netdev
  Cc: gordan.mihaljevic, tung.q.nguyen, hoang.h.le, jon.maloy,
	canh.d.luu, ying.xue, tipc-discussion
In-Reply-To: <1538159002-11800-1-git-send-email-jon.maloy@ericsson.com>

The function tipc_msg_reverse() is reversing the header of a message
while reusing the original buffer. We have seen at several occasions
that this may have unfortunate side effects when the buffer to be
reversed is a clone.

In one of the following commits we will again need to reverse cloned
buffers, so this is the right time to permanently eliminate this
problem. In this commit we let the said function always consume the
original buffer and replace it with a new one when applicable.

Acked-by: Ying Xue <ying.xue@windriver.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
 net/tipc/msg.c | 58 ++++++++++++++++++++++++++++------------------------------
 1 file changed, 28 insertions(+), 30 deletions(-)

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index b618910..00fbb5c 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -499,54 +499,52 @@ bool tipc_msg_make_bundle(struct sk_buff **skb,  struct tipc_msg *msg,
 /**
  * tipc_msg_reverse(): swap source and destination addresses and add error code
  * @own_node: originating node id for reversed message
- * @skb:  buffer containing message to be reversed; may be replaced.
+ * @skb:  buffer containing message to be reversed; will be consumed
  * @err:  error code to be set in message, if any
- * Consumes buffer at failure
+ * Replaces consumed buffer with new one when successful
  * Returns true if success, otherwise false
  */
 bool tipc_msg_reverse(u32 own_node,  struct sk_buff **skb, int err)
 {
 	struct sk_buff *_skb = *skb;
-	struct tipc_msg *hdr;
-	struct tipc_msg ohdr;
-	int dlen;
+	struct tipc_msg *_hdr, *hdr;
+	int hlen, dlen;
 
 	if (skb_linearize(_skb))
 		goto exit;
-	hdr = buf_msg(_skb);
-	dlen = min_t(uint, msg_data_sz(hdr), MAX_FORWARD_SIZE);
-	if (msg_dest_droppable(hdr))
+	_hdr = buf_msg(_skb);
+	dlen = min_t(uint, msg_data_sz(_hdr), MAX_FORWARD_SIZE);
+	hlen = msg_hdr_sz(_hdr);
+
+	if (msg_dest_droppable(_hdr))
 		goto exit;
-	if (msg_errcode(hdr))
+	if (msg_errcode(_hdr))
 		goto exit;
 
-	/* Take a copy of original header before altering message */
-	memcpy(&ohdr, hdr, msg_hdr_sz(hdr));
-
-	/* Never return SHORT header; expand by replacing buffer if necessary */
-	if (msg_short(hdr)) {
-		*skb = tipc_buf_acquire(BASIC_H_SIZE + dlen, GFP_ATOMIC);
-		if (!*skb)
-			goto exit;
-		memcpy((*skb)->data + BASIC_H_SIZE, msg_data(hdr), dlen);
-		kfree_skb(_skb);
-		_skb = *skb;
-		hdr = buf_msg(_skb);
-		memcpy(hdr, &ohdr, BASIC_H_SIZE);
-		msg_set_hdr_sz(hdr, BASIC_H_SIZE);
-	}
+	/* Never return SHORT header */
+	if (hlen == SHORT_H_SIZE)
+		hlen = BASIC_H_SIZE;
 
-	/* Now reverse the concerned fields */
+	/* Allocate new buffer to return */
+	*skb = tipc_buf_acquire(hlen + dlen, GFP_ATOMIC);
+	if (!*skb)
+		goto exit;
+	memcpy((*skb)->data, _skb->data, msg_hdr_sz(_hdr));
+	memcpy((*skb)->data + hlen, msg_data(_hdr), dlen);
+
+	/* Build reverse header in new buffer */
+	hdr = buf_msg(*skb);
+	msg_set_hdr_sz(hdr, hlen);
 	msg_set_errcode(hdr, err);
 	msg_set_non_seq(hdr, 0);
-	msg_set_origport(hdr, msg_destport(&ohdr));
-	msg_set_destport(hdr, msg_origport(&ohdr));
-	msg_set_destnode(hdr, msg_prevnode(&ohdr));
+	msg_set_origport(hdr, msg_destport(_hdr));
+	msg_set_destport(hdr, msg_origport(_hdr));
+	msg_set_destnode(hdr, msg_prevnode(_hdr));
 	msg_set_prevnode(hdr, own_node);
 	msg_set_orignode(hdr, own_node);
-	msg_set_size(hdr, msg_hdr_sz(hdr) + dlen);
-	skb_trim(_skb, msg_size(hdr));
+	msg_set_size(hdr, hlen + dlen);
 	skb_orphan(_skb);
+	kfree_skb(_skb);
 	return true;
 exit:
 	kfree_skb(_skb);
-- 
2.1.4

^ permalink raw reply related

* [net-next 0/5] tipc: make connection setup more robust
From: Jon Maloy @ 2018-09-28 18:23 UTC (permalink / raw)
  To: davem, netdev
  Cc: gordan.mihaljevic, tung.q.nguyen, hoang.h.le, jon.maloy,
	canh.d.luu, ying.xue, tipc-discussion

In this series we make a few improvements to the connection setup and
probing mechanism, culminating in the last commit where we make it 
possible for a client socket to make multiple setup attempts in case
it encounters receive buffer overflow at the listener socket.

Jon Maloy (4):
  tipc: refactor function tipc_msg_reverse()
  tipc: refactor function tipc_sk_timeout()
  tipc: refactor function tipc_sk_filter_connect()
  tipc: add SYN bit to connection setup messages

Tung Nguyen (1):
  tipc: buffer overflow handling in listener socket

 net/tipc/msg.c    |  78 ++++++++++++--------
 net/tipc/msg.h    |  11 +++
 net/tipc/node.h   |  12 ++--
 net/tipc/socket.c | 207 ++++++++++++++++++++++++++++++++----------------------
 4 files changed, 190 insertions(+), 118 deletions(-)

-- 
2.1.4

^ permalink raw reply

* [net-next 3/5] tipc: refactor function tipc_sk_filter_connect()
From: Jon Maloy @ 2018-09-28 18:23 UTC (permalink / raw)
  To: davem, netdev
  Cc: gordan.mihaljevic, tung.q.nguyen, hoang.h.le, jon.maloy,
	canh.d.luu, ying.xue, tipc-discussion
In-Reply-To: <1538159002-11800-1-git-send-email-jon.maloy@ericsson.com>

We refactor the function tipc_sk_filter_connect(), both to make it
more readable and as a preparation for the next commit.

Acked-by: Ying Xue <ying.xue@windriver.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
 net/tipc/socket.c | 101 +++++++++++++++++++++++-------------------------------
 1 file changed, 43 insertions(+), 58 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 8152a81..094cfdc 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1959,91 +1959,76 @@ static void tipc_sk_proto_rcv(struct sock *sk,
 }
 
 /**
- * tipc_filter_connect - Handle incoming message for a connection-based socket
+ * tipc_sk_filter_connect - check incoming message for a connection-based socket
  * @tsk: TIPC socket
- * @skb: pointer to message buffer. Set to NULL if buffer is consumed
- *
- * Returns true if everything ok, false otherwise
+ * @skb: pointer to message buffer.
+ * Returns true if message should be added to receive queue, false otherwise
  */
 static bool tipc_sk_filter_connect(struct tipc_sock *tsk, struct sk_buff *skb)
 {
 	struct sock *sk = &tsk->sk;
 	struct net *net = sock_net(sk);
 	struct tipc_msg *hdr = buf_msg(skb);
-	u32 pport = msg_origport(hdr);
-	u32 pnode = msg_orignode(hdr);
+	bool con_msg = msg_connected(hdr);
+	u32 pport = tsk_peer_port(tsk);
+	u32 pnode = tsk_peer_node(tsk);
+	u32 oport = msg_origport(hdr);
+	u32 onode = msg_orignode(hdr);
+	int err = msg_errcode(hdr);
 
 	if (unlikely(msg_mcast(hdr)))
 		return false;
 
 	switch (sk->sk_state) {
 	case TIPC_CONNECTING:
-		/* Accept only ACK or NACK message */
-		if (unlikely(!msg_connected(hdr))) {
-			if (pport != tsk_peer_port(tsk) ||
-			    pnode != tsk_peer_node(tsk))
-				return false;
-
-			tipc_set_sk_state(sk, TIPC_DISCONNECTING);
-			sk->sk_err = ECONNREFUSED;
-			sk->sk_state_change(sk);
-			return true;
-		}
-
-		if (unlikely(msg_errcode(hdr))) {
-			tipc_set_sk_state(sk, TIPC_DISCONNECTING);
-			sk->sk_err = ECONNREFUSED;
-			sk->sk_state_change(sk);
-			return true;
-		}
-
-		if (unlikely(!msg_isdata(hdr))) {
-			tipc_set_sk_state(sk, TIPC_DISCONNECTING);
-			sk->sk_err = EINVAL;
-			sk->sk_state_change(sk);
-			return true;
+		/* Setup ACK */
+		if (likely(con_msg)) {
+			if (err)
+				break;
+			tipc_sk_finish_conn(tsk, oport, onode);
+			msg_set_importance(&tsk->phdr, msg_importance(hdr));
+			/* ACK+ message with data is added to receive queue */
+			if (msg_data_sz(hdr))
+				return true;
+			/* Empty ACK-, - wake up sleeping connect() and drop */
+			sk->sk_data_ready(sk);
+			msg_set_dest_droppable(hdr, 1);
+			return false;
 		}
+		/* Ignore connectionless message if not from listening socket */
+		if (oport != pport || onode != pnode)
+			return false;
 
-		tipc_sk_finish_conn(tsk, msg_origport(hdr), msg_orignode(hdr));
-		msg_set_importance(&tsk->phdr, msg_importance(hdr));
-
-		/* If 'ACK+' message, add to socket receive queue */
-		if (msg_data_sz(hdr))
-			return true;
-
-		/* If empty 'ACK-' message, wake up sleeping connect() */
-		sk->sk_data_ready(sk);
-
-		/* 'ACK-' message is neither accepted nor rejected: */
-		msg_set_dest_droppable(hdr, 1);
-		return false;
-
+		/* Rejected SYN - abort */
+		break;
 	case TIPC_OPEN:
 	case TIPC_DISCONNECTING:
-		break;
+		return false;
 	case TIPC_LISTEN:
 		/* Accept only SYN message */
-		if (!msg_connected(hdr) && !(msg_errcode(hdr)))
+		if (!con_msg && !err)
 			return true;
-		break;
+		return false;
 	case TIPC_ESTABLISHED:
 		/* Accept only connection-based messages sent by peer */
-		if (unlikely(!tsk_peer_msg(tsk, hdr)))
+		if (likely(con_msg && !err && pport == oport && pnode == onode))
+			return true;
+		if (!tsk_peer_msg(tsk, hdr))
 			return false;
-
-		if (unlikely(msg_errcode(hdr))) {
-			tipc_set_sk_state(sk, TIPC_DISCONNECTING);
-			/* Let timer expire on it's own */
-			tipc_node_remove_conn(net, tsk_peer_node(tsk),
-					      tsk->portid);
-			sk->sk_state_change(sk);
-		}
+		if (!err)
+			return true;
+		tipc_set_sk_state(sk, TIPC_DISCONNECTING);
+		tipc_node_remove_conn(net, pnode, tsk->portid);
+		sk->sk_state_change(sk);
 		return true;
 	default:
 		pr_err("Unknown sk_state %u\n", sk->sk_state);
 	}
-
-	return false;
+	/* Abort connection setup attempt */
+	tipc_set_sk_state(sk, TIPC_DISCONNECTING);
+	sk->sk_err = ECONNREFUSED;
+	sk->sk_state_change(sk);
+	return true;
 }
 
 /**
-- 
2.1.4

^ permalink raw reply related

* [net-next 2/5] tipc: refactor function tipc_sk_timeout()
From: Jon Maloy @ 2018-09-28 18:23 UTC (permalink / raw)
  To: davem, netdev
  Cc: gordan.mihaljevic, tung.q.nguyen, hoang.h.le, jon.maloy,
	canh.d.luu, ying.xue, tipc-discussion
In-Reply-To: <1538159002-11800-1-git-send-email-jon.maloy@ericsson.com>

We refactor this function as a preparation for the coming commits in
the same series.

Acked-by: Ying Xue <ying.xue@windriver.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
 net/tipc/socket.c | 62 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 38 insertions(+), 24 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 3f03ddd..8152a81 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2545,43 +2545,57 @@ static int tipc_shutdown(struct socket *sock, int how)
 	return res;
 }
 
+static void tipc_sk_check_probing_state(struct sock *sk,
+					struct sk_buff_head *list)
+{
+	struct tipc_sock *tsk = tipc_sk(sk);
+	u32 pnode = tsk_peer_node(tsk);
+	u32 pport = tsk_peer_port(tsk);
+	u32 self = tsk_own_node(tsk);
+	u32 oport = tsk->portid;
+	struct sk_buff *skb;
+
+	if (tsk->probe_unacked) {
+		tipc_set_sk_state(sk, TIPC_DISCONNECTING);
+		sk->sk_err = ECONNABORTED;
+		tipc_node_remove_conn(sock_net(sk), pnode, pport);
+		sk->sk_state_change(sk);
+		return;
+	}
+	/* Prepare new probe */
+	skb = tipc_msg_create(CONN_MANAGER, CONN_PROBE, INT_H_SIZE, 0,
+			      pnode, self, pport, oport, TIPC_OK);
+	if (skb)
+		__skb_queue_tail(list, skb);
+	tsk->probe_unacked = true;
+	sk_reset_timer(sk, &sk->sk_timer, jiffies + CONN_PROBING_INTV);
+}
+
 static void tipc_sk_timeout(struct timer_list *t)
 {
 	struct sock *sk = from_timer(sk, t, sk_timer);
 	struct tipc_sock *tsk = tipc_sk(sk);
-	u32 peer_port = tsk_peer_port(tsk);
-	u32 peer_node = tsk_peer_node(tsk);
-	u32 own_node = tsk_own_node(tsk);
-	u32 own_port = tsk->portid;
-	struct net *net = sock_net(sk);
-	struct sk_buff *skb = NULL;
+	u32 pnode = tsk_peer_node(tsk);
+	struct sk_buff_head list;
 
+	skb_queue_head_init(&list);
 	bh_lock_sock(sk);
-	if (!tipc_sk_connected(sk))
-		goto exit;
 
 	/* Try again later if socket is busy */
 	if (sock_owned_by_user(sk)) {
 		sk_reset_timer(sk, &sk->sk_timer, jiffies + HZ / 20);
-		goto exit;
+		bh_unlock_sock(sk);
+		return;
 	}
 
-	if (tsk->probe_unacked) {
-		tipc_set_sk_state(sk, TIPC_DISCONNECTING);
-		tipc_node_remove_conn(net, peer_node, peer_port);
-		sk->sk_state_change(sk);
-		goto exit;
-	}
-	/* Send new probe */
-	skb = tipc_msg_create(CONN_MANAGER, CONN_PROBE, INT_H_SIZE, 0,
-			      peer_node, own_node, peer_port, own_port,
-			      TIPC_OK);
-	tsk->probe_unacked = true;
-	sk_reset_timer(sk, &sk->sk_timer, jiffies + CONN_PROBING_INTV);
-exit:
+	if (sk->sk_state == TIPC_ESTABLISHED)
+		tipc_sk_check_probing_state(sk, &list);
+
 	bh_unlock_sock(sk);
-	if (skb)
-		tipc_node_xmit_skb(net, skb, peer_node, own_port);
+
+	if (!skb_queue_empty(&list))
+		tipc_node_xmit(sock_net(sk), &list, pnode, tsk->portid);
+
 	sock_put(sk);
 }
 
-- 
2.1.4

^ permalink raw reply related

* [net-next 4/5] tipc: add SYN bit to connection setup messages
From: Jon Maloy @ 2018-09-28 18:23 UTC (permalink / raw)
  To: davem, netdev
  Cc: gordan.mihaljevic, tung.q.nguyen, hoang.h.le, jon.maloy,
	canh.d.luu, ying.xue, tipc-discussion
In-Reply-To: <1538159002-11800-1-git-send-email-jon.maloy@ericsson.com>

Messages intended for intitating a connection are currently
indistinguishable from regular datagram messages. The TIPC
protocol specification defines bit 17 in word 0 as a SYN bit
to allow sanity check of such messages in the listening socket,
but this has so far never been implemented.

We do that in this commit.

Acked-by: Ying Xue <ying.xue@windriver.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
 net/tipc/msg.h    | 10 ++++++++++
 net/tipc/node.h   | 12 +++++++-----
 net/tipc/socket.c |  5 +++++
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/net/tipc/msg.h b/net/tipc/msg.h
index a4e944d..cd64d7b 100644
--- a/net/tipc/msg.h
+++ b/net/tipc/msg.h
@@ -216,6 +216,16 @@ static inline void msg_set_non_seq(struct tipc_msg *m, u32 n)
 	msg_set_bits(m, 0, 20, 1, n);
 }
 
+static inline int msg_is_syn(struct tipc_msg *m)
+{
+	return msg_bits(m, 0, 17, 1);
+}
+
+static inline void msg_set_syn(struct tipc_msg *m, u32 d)
+{
+	msg_set_bits(m, 0, 17, 1, d);
+}
+
 static inline int msg_dest_droppable(struct tipc_msg *m)
 {
 	return msg_bits(m, 0, 19, 1);
diff --git a/net/tipc/node.h b/net/tipc/node.h
index 48b3298..03f5efb 100644
--- a/net/tipc/node.h
+++ b/net/tipc/node.h
@@ -45,6 +45,7 @@
 /* Optional capabilities supported by this code version
  */
 enum {
+	TIPC_SYN_BIT          = (1),
 	TIPC_BCAST_SYNCH      = (1 << 1),
 	TIPC_BCAST_STATE_NACK = (1 << 2),
 	TIPC_BLOCK_FLOWCTL    = (1 << 3),
@@ -53,11 +54,12 @@ enum {
 	TIPC_LINK_PROTO_SEQNO = (1 << 6)
 };
 
-#define TIPC_NODE_CAPABILITIES (TIPC_BCAST_SYNCH       |  \
-				TIPC_BCAST_STATE_NACK  |  \
-				TIPC_BCAST_RCAST       |  \
-				TIPC_BLOCK_FLOWCTL     |  \
-				TIPC_NODE_ID128        |  \
+#define TIPC_NODE_CAPABILITIES (TIPC_SYN_BIT           |  \
+				TIPC_BCAST_SYNCH       |   \
+				TIPC_BCAST_STATE_NACK  |   \
+				TIPC_BCAST_RCAST       |   \
+				TIPC_BLOCK_FLOWCTL     |   \
+				TIPC_NODE_ID128        |   \
 				TIPC_LINK_PROTO_SEQNO)
 #define INVALID_BEARER_ID -1
 
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 094cfdc..89d6dc0 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1319,6 +1319,7 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
 			tsk->conn_type = dest->addr.name.name.type;
 			tsk->conn_instance = dest->addr.name.name.instance;
 		}
+		msg_set_syn(hdr, 1);
 	}
 
 	seq = &dest->addr.nameseq;
@@ -1478,6 +1479,7 @@ static void tipc_sk_finish_conn(struct tipc_sock *tsk, u32 peer_port,
 	struct net *net = sock_net(sk);
 	struct tipc_msg *msg = &tsk->phdr;
 
+	msg_set_syn(msg, 0);
 	msg_set_destnode(msg, peer_node);
 	msg_set_destport(msg, peer_port);
 	msg_set_type(msg, TIPC_CONN_MSG);
@@ -2006,6 +2008,9 @@ static bool tipc_sk_filter_connect(struct tipc_sock *tsk, struct sk_buff *skb)
 		return false;
 	case TIPC_LISTEN:
 		/* Accept only SYN message */
+		if (!msg_is_syn(hdr) &&
+		    tipc_node_get_capabilities(net, onode) & TIPC_SYN_BIT)
+			return false;
 		if (!con_msg && !err)
 			return true;
 		return false;
-- 
2.1.4

^ permalink raw reply related

* bond: take rcu lock in bond_poll_controller
From: Dave Jones @ 2018-09-28 18:24 UTC (permalink / raw)
  To: netdev
In-Reply-To: <20180928161631.3pab3m2k6zvoemnz@codemonkey.org.uk>

Callers of bond_for_each_slave_rcu are expected to hold the rcu lock,
otherwise a trace like below is shown

WARNING: CPU: 2 PID: 179 at net/core/dev.c:6567 netdev_lower_get_next_private_rcu+0x34/0x40
CPU: 2 PID: 179 Comm: kworker/u16:15 Not tainted 4.19.0-rc5-backup+ #1
Workqueue: bond0 bond_mii_monitor
RIP: 0010:netdev_lower_get_next_private_rcu+0x34/0x40
Code: 48 89 fb e8 fe 29 63 ff 85 c0 74 1e 48 8b 45 00 48 81 c3 c0 00 00 00 48 8b 00 48 39 d8 74 0f 48 89 45 00 48 8b 40 f8 5b 5d c3 <0f> 0b eb de 31 c0 eb f5 0f 1f 40 00 0f 1f 44 00 00 48 8>
RSP: 0018:ffffc9000087fa68 EFLAGS: 00010046
RAX: 0000000000000000 RBX: ffff880429614560 RCX: 0000000000000000
RDX: 0000000000000001 RSI: 00000000ffffffff RDI: ffffffffa184ada0
RBP: ffffc9000087fa80 R08: 0000000000000001 R09: 0000000000000000
R10: ffffc9000087f9f0 R11: ffff880429798040 R12: ffff8804289d5980
R13: ffffffffa1511f60 R14: 00000000000000c8 R15: 00000000ffffffff
FS:  0000000000000000(0000) GS:ffff88042f880000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f4b78fce180 CR3: 000000018180f006 CR4: 00000000001606e0
Call Trace:
 bond_poll_controller+0x52/0x170
 netpoll_poll_dev+0x79/0x290
 netpoll_send_skb_on_dev+0x158/0x2c0
 netpoll_send_udp+0x2d5/0x430
 write_ext_msg+0x1e0/0x210
 console_unlock+0x3c4/0x630
 vprintk_emit+0xfa/0x2f0
 printk+0x52/0x6e
 ? __netdev_printk+0x12b/0x220
 netdev_info+0x64/0x80
 ? bond_3ad_set_carrier+0xe9/0x180
 bond_select_active_slave+0x1fc/0x310
 bond_mii_monitor+0x709/0x9b0
 process_one_work+0x221/0x5e0
 worker_thread+0x4f/0x3b0
 kthread+0x100/0x140
 ? process_one_work+0x5e0/0x5e0
 ? kthread_delayed_work_timer_fn+0x90/0x90
 ret_from_fork+0x24/0x30

Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Dave Jones <davej@codemonkey.org.uk>

-- 
v3: Do this in netpoll_send_skb_on_dev as Cong suggests.

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 3219a2932463..4f9494381635 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -330,6 +330,7 @@ void netpoll_send_skb_on_dev(struct netpoll *np, struct sk_buff *skb,
 	/* It is up to the caller to keep npinfo alive. */
 	struct netpoll_info *npinfo;
 
+	rcu_read_lock();
 	lockdep_assert_irqs_disabled();
 
 	npinfo = rcu_dereference_bh(np->dev->npinfo);
@@ -374,6 +375,7 @@ void netpoll_send_skb_on_dev(struct netpoll *np, struct sk_buff *skb,
 		skb_queue_tail(&npinfo->txq, skb);
 		schedule_delayed_work(&npinfo->tx_work,0);
 	}
+	rcu_read_unlock();
 }
 EXPORT_SYMBOL(netpoll_send_skb_on_dev);
 

^ permalink raw reply related

* Re: [Patch net-next v3] net_sched: change tcf_del_walker() to take idrinfo->lock
From: Cong Wang @ 2018-09-28 18:29 UTC (permalink / raw)
  To: Ido Schimmel
  Cc: Linux Kernel Network Developers, Jiri Pirko, Jamal Hadi Salim,
	Vlad Buslov
In-Reply-To: <20180928181114.GA28797@splinter>

On Fri, Sep 28, 2018 at 11:11 AM Ido Schimmel <idosch@idosch.org> wrote:
> I don't think this will work given the reference count already dropped
> to 0, which is why the template deletion function was invoked. I didn't
> test the patch, but I don't see what would prevent the chain from being
> freed.

Good catch! So I should just "move" tcf_chain_destroy() into cls_flower.

A patch is being compiled now.

^ permalink raw reply

* Re: bond: take rcu lock in bond_poll_controller
From: Eric Dumazet @ 2018-09-28 18:38 UTC (permalink / raw)
  To: Dave Jones, netdev
In-Reply-To: <20180928182410.slizlkx6fiwkyb76@codemonkey.org.uk>



On 09/28/2018 11:24 AM, Dave Jones wrote:
> Callers of bond_for_each_slave_rcu are expected to hold the rcu lock,
> otherwise a trace like below is shown
> 
> WARNING: CPU: 2 PID: 179 at net/core/dev.c:6567 netdev_lower_get_next_private_rcu+0x34/0x40
> CPU: 2 PID: 179 Comm: kworker/u16:15 Not tainted 4.19.0-rc5-backup+ #1
>
> 
> Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
> Signed-off-by: Dave Jones <davej@codemonkey.org.uk>
> 


You forgot to change patch title.

^ permalink raw reply

* Re: [PATCH RFC net-next 4/5] net/ipv4: Update inet_dump_ifaddr to support NLM_F_DUMP_PROPER_HDR
From: Christian Brauner @ 2018-09-28 18:41 UTC (permalink / raw)
  To: dsahern; +Cc: netdev, davem, jbenc, stephen, David Ahern
In-Reply-To: <20180928154502.9983-5-dsahern@kernel.org>

On Fri, Sep 28, 2018 at 08:45:01AM -0700, dsahern@kernel.org wrote:
> From: David Ahern <dsahern@gmail.com>
> 
> Update inet_dump_ifaddr to check for NLM_F_DUMP_PROPER_HDR in the netlink
> message header. If the flag is set, the dump request is expected to have
> an ifaddrmsg struct as the header potentially followed by one or more
> attributes. Any data passed in the header or as an attribute is taken as
> a request to influence the data returned. Only values suppored by the
> dump handler are allowed to be non-0 or set in the request. At the moment
> only the IFA_TARGET_NETNSID attribute is supported. Follow on patches
> will support for other fields (e.g., honor ifa_index and only return data
> for the given device index).
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
>  net/ipv4/devinet.c | 52 +++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 41 insertions(+), 11 deletions(-)
> 
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index 44d931a3cd50..1e06a21cd8f4 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -1661,15 +1661,15 @@ static int inet_fill_ifaddr(struct sk_buff *skb, struct in_ifaddr *ifa,
>  
>  static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
>  {
> +	struct netlink_ext_ack *extack = cb->extack;
> +	const struct nlmsghdr *nlh = cb->nlh;
>  	struct inet_fill_args fillargs = {
>  		.portid = NETLINK_CB(cb->skb).portid,
> -		.seq = cb->nlh->nlmsg_seq,
> +		.seq = nlh->nlmsg_seq,
>  		.event = RTM_NEWADDR,
> -		.flags = NLM_F_MULTI,
>  		.netnsid = -1,
>  	};
>  	struct net *net = sock_net(skb->sk);
> -	struct nlattr *tb[IFA_MAX+1];
>  	struct net *tgt_net = net;
>  	int h, s_h;
>  	int idx, s_idx;
> @@ -1683,15 +1683,45 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
>  	s_idx = idx = cb->args[1];
>  	s_ip_idx = ip_idx = cb->args[2];
>  
> -	if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
> -			ifa_ipv4_policy, NULL) >= 0) {
> -		if (tb[IFA_TARGET_NETNSID]) {
> -			fillargs.netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
> +	if (nlh->nlmsg_flags & NLM_F_DUMP_PROPER_HDR) {
> +		struct nlattr *tb[IFA_MAX+1];
> +		struct ifaddrmsg *ifm;
> +		int err, i;
> +
> +		if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) {
> +			NL_SET_ERR_MSG(extack, "Invalid header");
> +			return -EINVAL;
> +		}
> +
> +		ifm = (struct ifaddrmsg *) nlmsg_data(cb->nlh);
> +		if (ifm->ifa_prefixlen || ifm->ifa_flags || ifm->ifa_scope) {
> +			NL_SET_ERR_MSG(extack, "Invalid values in header for dump request");
> +			return -EINVAL;
> +		}
> +		if (ifm->ifa_index) {
> +			NL_SET_ERR_MSG(extack, "Filter by device index not supported");
> +			return -EINVAL;
> +		}
> +		err = nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
> +				ifa_ipv4_policy, NULL);
> +		if (err < 0)
> +			return err;
>  
> -			tgt_net = rtnl_get_net_ns_capable(skb->sk,
> -							  fillargs.netnsid);
> -			if (IS_ERR(tgt_net))
> -				return PTR_ERR(tgt_net);
> +		for (i = 0; i < IFA_MAX; ++i) {
> +			if (i == IFA_TARGET_NETNSID) {
> +				fillargs.netnsid = nla_get_s32(tb[i]);
> +
> +				tgt_net = rtnl_get_net_ns_capable(skb->sk,
> +								  fillargs.netnsid);
> +				if (IS_ERR(tgt_net))
> +					return PTR_ERR(tgt_net);
> +
> +				fillargs.flags |= NLM_F_DUMP_FILTERED;
> +			}
> +			if (tb[i]) {
> +				NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
> +				return -EINVAL;
> +			}

That loop doesn't do what it promises, no? Shouldn't it be:

		for (i = 0; i < IFA_MAX; ++i) {
			if (i == IFA_TARGET_NETNSID && tb[IFA_TARGET_NETNSID]) {
				fillargs.netnsid = nla_get_s32(tb[i]);

				tgt_net = rtnl_get_net_ns_capable(skb->sk,
								  fillargs.netnsid);
				if (IS_ERR(tgt_net))
					return PTR_ERR(tgt_net);

				fillargs.flags |= NLM_F_DUMP_FILTERED;
				continue;
			}

			if (tb[i]) {
				NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
				return -EINVAL;
			}
		}

>  	}
>  
> -- 
> 2.11.0
> 

^ permalink raw reply

* Re: [PATCH RFC net-next 5/5] net/ipv6: Update inet6_dump_addr to support NLM_F_DUMP_PROPER_HDR
From: Christian Brauner @ 2018-09-28 18:42 UTC (permalink / raw)
  To: dsahern; +Cc: netdev, davem, jbenc, stephen, David Ahern
In-Reply-To: <20180928154502.9983-6-dsahern@kernel.org>

On Fri, Sep 28, 2018 at 08:45:02AM -0700, dsahern@kernel.org wrote:
> From: David Ahern <dsahern@gmail.com>
> 
> Update inet6_dump_addr to check for NLM_F_DUMP_PROPER_HDR in the netlink
> message header. If the flag is set, the dump request is expected to have
> an ifaddrmsg struct as the header potentially followed by one or more
> attributes. Any data passed in the header or as an attribute is taken as
> a request to influence the data returned. Only values suppored by the
> dump handler are allowed to be non-0 or set in the request. At the moment
> only the IFA_TARGET_NETNSID attribute is supported. Follow on patches
> will support for other fields (e.g., honor ifa_index and only return data
> for the given device index).
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
>  net/ipv6/addrconf.c | 50 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 40 insertions(+), 10 deletions(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 375ea9d9869b..888e5a4b8dd2 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -5001,6 +5001,8 @@ static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,
>  static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
>  			   enum addr_type_t type)
>  {
> +	struct netlink_ext_ack *extack = cb->extack;
> +	const struct nlmsghdr *nlh = cb->nlh;
>  	struct inet6_fill_args fillargs = {
>  		.portid = NETLINK_CB(cb->skb).portid,
>  		.seq = cb->nlh->nlmsg_seq,
> @@ -5009,7 +5011,6 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
>  		.type = type,
>  	};
>  	struct net *net = sock_net(skb->sk);
> -	struct nlattr *tb[IFA_MAX+1];
>  	struct net *tgt_net = net;
>  	int h, s_h;
>  	int idx, ip_idx;
> @@ -5022,17 +5023,46 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
>  	s_idx = idx = cb->args[1];
>  	s_ip_idx = ip_idx = cb->args[2];
>  
> -	if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
> -			ifa_ipv6_policy, NULL) >= 0) {
> -		if (tb[IFA_TARGET_NETNSID]) {
> -			fillargs.netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
> +	if (nlh->nlmsg_flags & NLM_F_DUMP_PROPER_HDR) {
> +		struct nlattr *tb[IFA_MAX+1];
> +		struct ifaddrmsg *ifm;
> +		int err, i;
> +
> +		if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) {
> +			NL_SET_ERR_MSG(extack, "Invalid header");
> +			return -EINVAL;
> +		}
>  
> -			tgt_net = rtnl_get_net_ns_capable(skb->sk,
> -							  fillargs.netnsid);
> -			if (IS_ERR(tgt_net))
> -				return PTR_ERR(tgt_net);
> +		ifm = (struct ifaddrmsg *)nlmsg_data(cb->nlh);
> +		if (ifm->ifa_prefixlen || ifm->ifa_flags || ifm->ifa_scope) {
> +			NL_SET_ERR_MSG(extack, "Invalid values in header for dump request");
> +			return -EINVAL;
> +		}
> +		if (ifm->ifa_index) {
> +			NL_SET_ERR_MSG(extack, "Filter by device index not supported");
> +			return -EINVAL;
> +		}
>  
> -			fillargs.flags |= NLM_F_DUMP_FILTERED;
> +		err = nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
> +				  ifa_ipv6_policy, NULL);
> +		if (err < 0)
> +			return err;
> +
> +		for (i = 0; i < IFA_MAX; ++i) {
> +			if (i == IFA_TARGET_NETNSID) {
> +				fillargs.netnsid = nla_get_s32(tb[i]);
> +
> +				tgt_net = rtnl_get_net_ns_capable(skb->sk,
> +								  fillargs.netnsid);
> +				if (IS_ERR(tgt_net))
> +					return PTR_ERR(tgt_net);
> +
> +				fillargs.flags |= NLM_F_DUMP_FILTERED;
> +			}
> +			if (tb[i]) {
> +				NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
> +				return -EINVAL;
> +			}

Same comment/question as for ipv4. Shouldn't it be:


   	for (i = 0; i < IFA_MAX; ++i) {
   		if (i == IFA_TARGET_NETNSID && tb[i]) {
   			fillargs.netnsid = nla_get_s32(tb[i]);

   			tgt_net = rtnl_get_net_ns_capable(skb->sk,
   							  fillargs.netnsid);
   			if (IS_ERR(tgt_net))
   				return PTR_ERR(tgt_net);

   			fillargs.flags |= NLM_F_DUMP_FILTERED;
			continue;
   		}
   		if (tb[i]) {
   			NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
   			return -EINVAL;
   		}
>  		}
>  	}
>  
> -- 
> 2.11.0
> 

^ permalink raw reply

* Re: [PATCH RFC net-next 4/5] net/ipv4: Update inet_dump_ifaddr to support NLM_F_DUMP_PROPER_HDR
From: David Ahern @ 2018-09-28 18:43 UTC (permalink / raw)
  To: Christian Brauner, dsahern; +Cc: netdev, davem, jbenc, stephen
In-Reply-To: <20180928184115.n7xotgooia6xjfkh@brauner.io>

On 9/28/18 12:41 PM, Christian Brauner wrote:
>> @@ -1683,15 +1683,45 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
>>  	s_idx = idx = cb->args[1];
>>  	s_ip_idx = ip_idx = cb->args[2];
>>  
>> -	if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
>> -			ifa_ipv4_policy, NULL) >= 0) {
>> -		if (tb[IFA_TARGET_NETNSID]) {
>> -			fillargs.netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
>> +	if (nlh->nlmsg_flags & NLM_F_DUMP_PROPER_HDR) {
>> +		struct nlattr *tb[IFA_MAX+1];
>> +		struct ifaddrmsg *ifm;
>> +		int err, i;
>> +
>> +		if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) {
>> +			NL_SET_ERR_MSG(extack, "Invalid header");
>> +			return -EINVAL;
>> +		}
>> +
>> +		ifm = (struct ifaddrmsg *) nlmsg_data(cb->nlh);
>> +		if (ifm->ifa_prefixlen || ifm->ifa_flags || ifm->ifa_scope) {
>> +			NL_SET_ERR_MSG(extack, "Invalid values in header for dump request");
>> +			return -EINVAL;
>> +		}
>> +		if (ifm->ifa_index) {
>> +			NL_SET_ERR_MSG(extack, "Filter by device index not supported");
>> +			return -EINVAL;
>> +		}
>> +		err = nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
>> +				ifa_ipv4_policy, NULL);
>> +		if (err < 0)
>> +			return err;
>>  
>> -			tgt_net = rtnl_get_net_ns_capable(skb->sk,
>> -							  fillargs.netnsid);
>> -			if (IS_ERR(tgt_net))
>> -				return PTR_ERR(tgt_net);
>> +		for (i = 0; i < IFA_MAX; ++i) {
>> +			if (i == IFA_TARGET_NETNSID) {
>> +				fillargs.netnsid = nla_get_s32(tb[i]);
>> +
>> +				tgt_net = rtnl_get_net_ns_capable(skb->sk,
>> +								  fillargs.netnsid);
>> +				if (IS_ERR(tgt_net))
>> +					return PTR_ERR(tgt_net);
>> +
>> +				fillargs.flags |= NLM_F_DUMP_FILTERED;
>> +			}
>> +			if (tb[i]) {
>> +				NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
>> +				return -EINVAL;
>> +			}
> 
> That loop doesn't do what it promises, no? Shouldn't it be:

your right, that should be:
			} else if (tb[i]) {

^ permalink raw reply

* Re: [PATCH RFC net-next 1/5] net/netlink: Pass extack to dump callbacks
From: Christian Brauner @ 2018-09-28 18:43 UTC (permalink / raw)
  To: dsahern; +Cc: netdev, davem, jbenc, stephen, David Ahern
In-Reply-To: <20180928154502.9983-2-dsahern@kernel.org>

On Fri, Sep 28, 2018 at 08:44:58AM -0700, dsahern@kernel.org wrote:
> From: David Ahern <dsahern@gmail.com>
> 
> Pass extack to dump callbacks by adding extack to netlink_dump_control
> and transferring to netlink_callback. Update rtnetlink as the first
> user.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>

I like the idea of passing down extack.

Acked-by: Christian Brauner <christian@brauner.io>

> ---
>  include/linux/netlink.h  | 2 ++
>  net/core/rtnetlink.c     | 1 +
>  net/netlink/af_netlink.c | 1 +
>  3 files changed, 4 insertions(+)
> 
> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index 71f121b66ca8..8fc90308a653 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -176,6 +176,7 @@ struct netlink_callback {
>  	void			*data;
>  	/* the module that dump function belong to */
>  	struct module		*module;
> +	struct netlink_ext_ack	*extack;
>  	u16			family;
>  	u16			min_dump_alloc;
>  	unsigned int		prev_seq, seq;
> @@ -197,6 +198,7 @@ struct netlink_dump_control {
>  	int (*done)(struct netlink_callback *);
>  	void *data;
>  	struct module *module;
> +	struct netlink_ext_ack *extack;
>  	u16 min_dump_alloc;
>  };
>  
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 35162e1b06ad..da91b38297d3 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4689,6 +4689,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
>  				.dump		= dumpit,
>  				.min_dump_alloc	= min_dump_alloc,
>  				.module		= owner,
> +				.extack		= extack
>  			};
>  			err = netlink_dump_start(rtnl, skb, nlh, &c);
>  			/* netlink_dump_start() will keep a reference on
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index e3a0538ec0be..7d9e735b32c4 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -2307,6 +2307,7 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
>  	cb->module = control->module;
>  	cb->min_dump_alloc = control->min_dump_alloc;
>  	cb->skb = skb;
> +	cb->extack = control->extack;
>  
>  	if (control->start) {
>  		ret = control->start(cb);
> -- 
> 2.11.0
> 

^ permalink raw reply

* Re: [PATCH RFC net-next 2/5] net/ipv6: Refactor address dump to push inet6_fill_args to in6_dump_addrs
From: Christian Brauner @ 2018-09-28 18:44 UTC (permalink / raw)
  To: dsahern; +Cc: netdev, davem, jbenc, stephen, David Ahern
In-Reply-To: <20180928154502.9983-3-dsahern@kernel.org>

On Fri, Sep 28, 2018 at 08:44:59AM -0700, dsahern@kernel.org wrote:
> From: David Ahern <dsahern@gmail.com>
> 
> Pull the inet6_fill_args arg up to in6_dump_addrs and move netnsid
> into it. Since IFA_TARGET_NETNSID is a kernel side filter add the
> NLM_F_DUMP_FILTERED flag so userspace knows the request was honored.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>

Acked-by: Christian Brauner <christian@brauner.io>

> ---
>  net/ipv6/addrconf.c | 59 +++++++++++++++++++++++++++++------------------------
>  1 file changed, 32 insertions(+), 27 deletions(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index a9a317322388..375ea9d9869b 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -4793,12 +4793,19 @@ static inline int inet6_ifaddr_msgsize(void)
>  	       + nla_total_size(4)  /* IFA_RT_PRIORITY */;
>  }
>  
> +enum addr_type_t {
> +	UNICAST_ADDR,
> +	MULTICAST_ADDR,
> +	ANYCAST_ADDR,
> +};
> +
>  struct inet6_fill_args {
>  	u32 portid;
>  	u32 seq;
>  	int event;
>  	unsigned int flags;
>  	int netnsid;
> +	enum addr_type_t type;
>  };
>  
>  static int inet6_fill_ifaddr(struct sk_buff *skb, struct inet6_ifaddr *ifa,
> @@ -4930,39 +4937,28 @@ static int inet6_fill_ifacaddr(struct sk_buff *skb, struct ifacaddr6 *ifaca,
>  	return 0;
>  }
>  
> -enum addr_type_t {
> -	UNICAST_ADDR,
> -	MULTICAST_ADDR,
> -	ANYCAST_ADDR,
> -};
> -
>  /* called with rcu_read_lock() */
>  static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,
> -			  struct netlink_callback *cb, enum addr_type_t type,
> -			  int s_ip_idx, int *p_ip_idx, int netnsid)
> +			  struct netlink_callback *cb,
> +			  int s_ip_idx, int *p_ip_idx,
> +			  struct inet6_fill_args *fillargs)
>  {
> -	struct inet6_fill_args fillargs = {
> -		.portid = NETLINK_CB(cb->skb).portid,
> -		.seq = cb->nlh->nlmsg_seq,
> -		.flags = NLM_F_MULTI,
> -		.netnsid = netnsid,
> -	};
>  	struct ifmcaddr6 *ifmca;
>  	struct ifacaddr6 *ifaca;
>  	int err = 1;
>  	int ip_idx = *p_ip_idx;
>  
>  	read_lock_bh(&idev->lock);
> -	switch (type) {
> +	switch (fillargs->type) {
>  	case UNICAST_ADDR: {
>  		struct inet6_ifaddr *ifa;
> -		fillargs.event = RTM_NEWADDR;
> +		fillargs->event = RTM_NEWADDR;
>  
>  		/* unicast address incl. temp addr */
>  		list_for_each_entry(ifa, &idev->addr_list, if_list) {
>  			if (++ip_idx < s_ip_idx)
>  				continue;
> -			err = inet6_fill_ifaddr(skb, ifa, &fillargs);
> +			err = inet6_fill_ifaddr(skb, ifa, fillargs);
>  			if (err < 0)
>  				break;
>  			nl_dump_check_consistent(cb, nlmsg_hdr(skb));
> @@ -4970,26 +4966,26 @@ static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,
>  		break;
>  	}
>  	case MULTICAST_ADDR:
> -		fillargs.event = RTM_GETMULTICAST;
> +		fillargs->event = RTM_GETMULTICAST;
>  
>  		/* multicast address */
>  		for (ifmca = idev->mc_list; ifmca;
>  		     ifmca = ifmca->next, ip_idx++) {
>  			if (ip_idx < s_ip_idx)
>  				continue;
> -			err = inet6_fill_ifmcaddr(skb, ifmca, &fillargs);
> +			err = inet6_fill_ifmcaddr(skb, ifmca, fillargs);
>  			if (err < 0)
>  				break;
>  		}
>  		break;
>  	case ANYCAST_ADDR:
> -		fillargs.event = RTM_GETANYCAST;
> +		fillargs->event = RTM_GETANYCAST;
>  		/* anycast address */
>  		for (ifaca = idev->ac_list; ifaca;
>  		     ifaca = ifaca->aca_next, ip_idx++) {
>  			if (ip_idx < s_ip_idx)
>  				continue;
> -			err = inet6_fill_ifacaddr(skb, ifaca, &fillargs);
> +			err = inet6_fill_ifacaddr(skb, ifaca, fillargs);
>  			if (err < 0)
>  				break;
>  		}
> @@ -5005,10 +5001,16 @@ static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,
>  static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
>  			   enum addr_type_t type)
>  {
> +	struct inet6_fill_args fillargs = {
> +		.portid = NETLINK_CB(cb->skb).portid,
> +		.seq = cb->nlh->nlmsg_seq,
> +		.flags = NLM_F_MULTI,
> +		.netnsid = -1,
> +		.type = type,
> +	};
>  	struct net *net = sock_net(skb->sk);
>  	struct nlattr *tb[IFA_MAX+1];
>  	struct net *tgt_net = net;
> -	int netnsid = -1;
>  	int h, s_h;
>  	int idx, ip_idx;
>  	int s_idx, s_ip_idx;
> @@ -5023,11 +5025,14 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
>  	if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
>  			ifa_ipv6_policy, NULL) >= 0) {
>  		if (tb[IFA_TARGET_NETNSID]) {
> -			netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
> +			fillargs.netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
>  
> -			tgt_net = rtnl_get_net_ns_capable(skb->sk, netnsid);
> +			tgt_net = rtnl_get_net_ns_capable(skb->sk,
> +							  fillargs.netnsid);
>  			if (IS_ERR(tgt_net))
>  				return PTR_ERR(tgt_net);
> +
> +			fillargs.flags |= NLM_F_DUMP_FILTERED;
>  		}
>  	}
>  
> @@ -5046,8 +5051,8 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
>  			if (!idev)
>  				goto cont;
>  
> -			if (in6_dump_addrs(idev, skb, cb, type,
> -					   s_ip_idx, &ip_idx, netnsid) < 0)
> +			if (in6_dump_addrs(idev, skb, cb, s_ip_idx, &ip_idx,
> +					   &fillargs) < 0)
>  				goto done;
>  cont:
>  			idx++;
> @@ -5058,7 +5063,7 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
>  	cb->args[0] = h;
>  	cb->args[1] = idx;
>  	cb->args[2] = ip_idx;
> -	if (netnsid >= 0)
> +	if (fillargs.netnsid >= 0)
>  		put_net(tgt_net);
>  
>  	return skb->len;
> -- 
> 2.11.0
> 

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox