Netdev List
 help / color / mirror / Atom feed
* [PATCH net-next 1/4] qed: Add iWARP enablement support
From: Michal Kalderon @ 2017-09-19 17:26 UTC (permalink / raw)
  To: davem, netdev; +Cc: linux-rdma, dledford, Michal Kalderon, Ariel Elior
In-Reply-To: <1505841979-26365-1-git-send-email-Michal.Kalderon@cavium.com>

This patch is the last of the initial iWARP patch series. It
adds the possiblity to actually detect iWARP from the device and enable
it in the critical locations which basically make iWARP available.

It wasn't submitted until now as iWARP hadn't been accepted into
the rdma tree.

Signed-off-by: Michal Kalderon <Michal.Kalderon@cavium.com>
Signed-off-by: Ariel Elior <Ariel.Elior@cavium.com>
---
 drivers/net/ethernet/qlogic/qed/qed_cxt.c         |  6 ++++++
 drivers/net/ethernet/qlogic/qed/qed_mcp.c         | 10 +++++-----
 drivers/net/ethernet/qlogic/qed/qed_rdma.c        |  5 ++++-
 drivers/net/ethernet/qlogic/qed/qed_sp_commands.c |  1 +
 4 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_cxt.c b/drivers/net/ethernet/qlogic/qed/qed_cxt.c
index af106be..afd07ad 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_cxt.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_cxt.c
@@ -2069,6 +2069,12 @@ static void qed_rdma_set_pf_params(struct qed_hwfn *p_hwfn,
 
 	num_srqs = min_t(u32, 32 * 1024, p_params->num_srqs);
 
+	if (p_hwfn->mcp_info->func_info.protocol == QED_PCI_ETH_RDMA) {
+		DP_NOTICE(p_hwfn,
+			  "Current day drivers don't support RoCE & iWARP simultaneously on the same PF. Default to RoCE-only\n");
+		p_hwfn->hw_info.personality = QED_PCI_ETH_ROCE;
+	}
+
 	switch (p_hwfn->hw_info.personality) {
 	case QED_PCI_ETH_IWARP:
 		/* Each QP requires one connection */
diff --git a/drivers/net/ethernet/qlogic/qed/qed_mcp.c b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
index 376485d..8b99c7d 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
@@ -1691,12 +1691,12 @@ int qed_mcp_get_media_type(struct qed_dev *cdev, u32 *p_media_type)
 	case FW_MB_PARAM_GET_PF_RDMA_ROCE:
 		*p_proto = QED_PCI_ETH_ROCE;
 		break;
-	case FW_MB_PARAM_GET_PF_RDMA_BOTH:
-		DP_NOTICE(p_hwfn,
-			  "Current day drivers don't support RoCE & iWARP. Default to RoCE-only\n");
-		*p_proto = QED_PCI_ETH_ROCE;
-		break;
 	case FW_MB_PARAM_GET_PF_RDMA_IWARP:
+		*p_proto = QED_PCI_ETH_IWARP;
+		break;
+	case FW_MB_PARAM_GET_PF_RDMA_BOTH:
+		*p_proto = QED_PCI_ETH_RDMA;
+		break;
 	default:
 		DP_NOTICE(p_hwfn,
 			  "MFW answers GET_PF_RDMA_PROTOCOL but param is %08x\n",
diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
index 6fb9951..06715f7 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_rdma.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
@@ -156,7 +156,10 @@ static int qed_rdma_alloc(struct qed_hwfn *p_hwfn,
 		return rc;
 
 	p_hwfn->p_rdma_info = p_rdma_info;
-	p_rdma_info->proto = PROTOCOLID_ROCE;
+	if (QED_IS_IWARP_PERSONALITY(p_hwfn))
+		p_rdma_info->proto = PROTOCOLID_IWARP;
+	else
+		p_rdma_info->proto = PROTOCOLID_ROCE;
 
 	num_cons = qed_cxt_get_proto_cid_count(p_hwfn, p_rdma_info->proto,
 					       NULL);
diff --git a/drivers/net/ethernet/qlogic/qed/qed_sp_commands.c b/drivers/net/ethernet/qlogic/qed/qed_sp_commands.c
index 46d0c3c..a1d33f3 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_sp_commands.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_sp_commands.c
@@ -377,6 +377,7 @@ int qed_sp_pf_start(struct qed_hwfn *p_hwfn,
 		p_ramrod->personality = PERSONALITY_ISCSI;
 		break;
 	case QED_PCI_ETH_ROCE:
+	case QED_PCI_ETH_IWARP:
 		p_ramrod->personality = PERSONALITY_RDMA_AND_ETH;
 		break;
 	default:
-- 
1.8.3.1

^ permalink raw reply related

* [PATCH net-next 0/4] qed: iWARP fixes and enhancements
From: Michal Kalderon @ 2017-09-19 17:26 UTC (permalink / raw)
  To: davem-fT/PcQaiUtIeIZ0/mPfg9Q, netdev-u79uwXL29TY76Z2rM5mHXA
  Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA,
	dledford-H+wXaHxf7aLQT0dZR+AlfA, Michal Kalderon, Ariel Elior

This patch series includes several fixes and enhancements
related to iWARP.

Patch #1 is actually the last of the initial iWARP submission.
It has been delayed until now as I wanted to make sure that qedr
supports iWARP prior to enabling iWARP device detection.

iWARP changes in RDMA tree have been accepted and targeted at
kernel 4.15, therefore, all iWARP fixes for this cycle are
submitted to net-next.

Signed-off by: Michal.Kalderon-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org
Signed-off-by: Ariel Elior <Ariel.Elior-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>

Michal Kalderon (4):
  qed: Add iWARP enablement support
  qed: Add iWARP out of order support
  qed: Fix maximum number of CQs for iWARP
  qed: iWARP - Add check for errors on a SYN packet

 drivers/net/ethernet/qlogic/qed/qed_cxt.c         |  6 +++
 drivers/net/ethernet/qlogic/qed/qed_iwarp.c       | 52 +++++++++++++++++++++++
 drivers/net/ethernet/qlogic/qed/qed_iwarp.h       | 11 ++++-
 drivers/net/ethernet/qlogic/qed/qed_ll2.c         |  1 +
 drivers/net/ethernet/qlogic/qed/qed_mcp.c         | 10 ++---
 drivers/net/ethernet/qlogic/qed/qed_rdma.c        | 24 +++++++----
 drivers/net/ethernet/qlogic/qed/qed_sp_commands.c |  1 +
 include/linux/qed/qed_ll2_if.h                    |  1 +
 8 files changed, 91 insertions(+), 15 deletions(-)

-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [PATCH] net: compat: assert the size of cmsg copied in is as expected
From: Meng Xu @ 2017-09-19 17:19 UTC (permalink / raw)
  To: davem, netdev, linux-kernel; +Cc: meng.xu, sanidhya, taesoo, Meng Xu

The actual length of cmsg fetched in during the second loop
(i.e., kcmsg - kcmsg_base) could be different from what we
get from the first loop (i.e., kcmlen).

The main reason is that the two get_user() calls in the two
loops (i.e., get_user(ucmlen, &ucmsg->cmsg_len) and
__get_user(ucmlen, &ucmsg->cmsg_len)) could cause ucmlen
to have different values even they fetch from the same userspace
address, as user can race to change the memory content in
&ucmsg->cmsg_len across fetches.

Although in the second loop, the sanity check
if ((char *)kcmsg_base + kcmlen - (char *)kcmsg < CMSG_ALIGN(tmp))
is inplace, it only ensures that the cmsg fetched in during the
second loop does not exceed the length of kcmlen, but not
necessarily equal to kcmlen. But indicated by the assignment
kmsg->msg_controllen = kcmlen, we should enforce that.

This patch adds this additional sanity check and ensures that
what is recorded in kmsg->msg_controllen is the actual cmsg length.

Signed-off-by: Meng Xu <mengxu.gatech@gmail.com>
---
 net/compat.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/compat.c b/net/compat.c
index 6ded6c8..2238171 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -185,6 +185,13 @@ int cmsghdr_from_user_compat_to_kern(struct msghdr *kmsg, struct sock *sk,
 		ucmsg = cmsg_compat_nxthdr(kmsg, ucmsg, ucmlen);
 	}
 
+	/*
+	 * check the length of messages copied in is the same as the
+	 * what we get from the first loop
+	 */
+	if ((char *)kcmsg - (char *)kcmsg_base != kcmlen)
+		goto Einval;
+
 	/* Ok, looks like we made it.  Hook it up and return success. */
 	kmsg->msg_control = kcmsg_base;
 	kmsg->msg_controllen = kcmlen;
-- 
2.7.4

^ permalink raw reply related

* [PATCH net] tcp: fastopen: fix on syn-data transmit failure
From: Eric Dumazet @ 2017-09-19 17:05 UTC (permalink / raw)
  To: David Miller; +Cc: Yuchung Cheng, Neal Cardwell, netdev

From: Eric Dumazet <edumazet@google.com>

Our recent change exposed a bug in TCP Fastopen Client that syzkaller
found right away [1]

When we prepare skb with SYN+DATA, we attempt to transmit it,
and we update socket state as if the transmit was a success.

In socket RTX queue we have two skbs, one with the SYN alone,
and a second one containing the DATA.

When (malicious) ACK comes in, we now complain that second one had no
skb_mstamp.

The proper fix is to make sure that if the transmit failed, we do not
pretend we sent the DATA skb, and make it our send_head.

When 3WHS completes, we can now send the DATA right away, without having
to wait for a timeout.

[1]
WARNING: CPU: 0 PID: 100189 at net/ipv4/tcp_input.c:3117 tcp_clean_rtx_queue+0x2057/0x2ab0 net/ipv4/tcp_input.c:3117()

 WARN_ON_ONCE(last_ackt == 0);

Modules linked in:
CPU: 0 PID: 100189 Comm: syz-executor1 Not tainted 
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
 0000000000000000 ffff8800b35cb1d8 ffffffff81cad00d 0000000000000000
 ffffffff828a4347 ffff88009f86c080 ffffffff8316eb20 0000000000000d7f
 ffff8800b35cb220 ffffffff812c33c2 ffff8800baad2440 00000009d46575c0
Call Trace:
 [<ffffffff81cad00d>] __dump_stack 
 [<ffffffff81cad00d>] dump_stack+0xc1/0x124 
 [<ffffffff812c33c2>] warn_slowpath_common+0xe2/0x150 
 [<ffffffff812c361e>] warn_slowpath_null+0x2e/0x40 
 [<ffffffff828a4347>] tcp_clean_rtx_queue+0x2057/0x2ab0 n
 [<ffffffff828ae6fd>] tcp_ack+0x151d/0x3930 
 [<ffffffff828baa09>] tcp_rcv_state_process+0x1c69/0x4fd0 
 [<ffffffff828efb7f>] tcp_v4_do_rcv+0x54f/0x7c0 
 [<ffffffff8258aacb>] sk_backlog_rcv 
 [<ffffffff8258aacb>] __release_sock+0x12b/0x3a0 
 [<ffffffff8258ad9e>] release_sock+0x5e/0x1c0 
 [<ffffffff8294a785>] inet_wait_for_connect 
 [<ffffffff8294a785>] __inet_stream_connect+0x545/0xc50 
 [<ffffffff82886f08>] tcp_sendmsg_fastopen 
 [<ffffffff82886f08>] tcp_sendmsg+0x2298/0x35a0 
 [<ffffffff82952515>] inet_sendmsg+0xe5/0x520 
 [<ffffffff8257152f>] sock_sendmsg_nosec 
 [<ffffffff8257152f>] sock_sendmsg+0xcf/0x110 

Fixes: 8c72c65b426b ("tcp: update skb->skb_mstamp more carefully")
Fixes: 783237e8daf1 ("net-tcp: Fast Open client - sending SYN-data")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
---
 net/ipv4/tcp_output.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 517d737059d18d8821b65dcdf54d9bb3448784c2..0bc9e46a53696578eb6e911f2f75e6b34c80894f 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -3389,6 +3389,10 @@ static int tcp_send_syn_data(struct sock *sk, struct sk_buff *syn)
 		goto done;
 	}
 
+	/* data was not sent, this is our new send_head */
+	sk->sk_send_head = syn_data;
+	tp->packets_out -= tcp_skb_pcount(syn_data);
+
 fallback:
 	/* Send a regular SYN with Fast Open cookie request option */
 	if (fo->cookie.len > 0)
@@ -3441,6 +3445,11 @@ int tcp_connect(struct sock *sk)
 	 */
 	tp->snd_nxt = tp->write_seq;
 	tp->pushed_seq = tp->write_seq;
+	buff = tcp_send_head(sk);
+	if (unlikely(buff)) {
+		tp->snd_nxt	= TCP_SKB_CB(buff)->seq;
+		tp->pushed_seq	= TCP_SKB_CB(buff)->seq;
+	}
 	TCP_INC_STATS(sock_net(sk), TCP_MIB_ACTIVEOPENS);
 
 	/* Timer for repeating the SYN until an answer. */

^ permalink raw reply related

* Re: [PATCH RFC V1 net-next 0/6] Time based packet transmission
From: Richard Cochran @ 2017-09-19 16:46 UTC (permalink / raw)
  To: Miroslav Lichvar
  Cc: Richard Cochran, netdev, Andre Guedes, Henrik Austad,
	linux-kernel, Jesus Sanchez-Palencia, intel-wired-lan,
	John Stultz, Thomas Gleixner, Anna-Maria Gleixner, David Miller
In-Reply-To: <20170919144302.GB4347@localhost>

On Tue, Sep 19, 2017 at 04:43:02PM +0200, Miroslav Lichvar wrote:
> If I understand it correctly, this also allows us to make a PTP/NTP
> "one-step" clock with HW that doesn't support it directly.

Cool, yeah, I hadn't thought of that, but it would work...

Thanks,
Richard

^ permalink raw reply

* Re: [PATCH net v2] l2tp: fix race condition in l2tp_tunnel_delete
From: Guillaume Nault @ 2017-09-19 16:43 UTC (permalink / raw)
  To: Sabrina Dubroca; +Cc: netdev, Xin Long, Tom Parkin
In-Reply-To: <6bfc5aceda47773af4c75fe7e0e3c0d255a2342d.1505828155.git.sd@queasysnail.net>

On Tue, Sep 19, 2017 at 03:40:40PM +0200, Sabrina Dubroca wrote:
> If we try to delete the same tunnel twice, the first delete operation
> does a lookup (l2tp_tunnel_get), finds the tunnel, calls
> l2tp_tunnel_delete, which queues it for deletion by
> l2tp_tunnel_del_work.
> 
> The second delete operation also finds the tunnel and calls
> l2tp_tunnel_delete. If the workqueue has already fired and started
> running l2tp_tunnel_del_work, then l2tp_tunnel_delete will queue the
> same tunnel a second time, and try to free the socket again.
> 
> Add a dead flag to prevent firing the workqueue twice. Then we can
> remove the check of queue_work's result that was meant to prevent that
> race but doesn't.
> 
> Also check the flag in the tunnel lookup functions, to avoid returning a
> tunnel that is already scheduled for destruction.
> 
> Reproducer:
> 
>     ip l2tp add tunnel tunnel_id 3000 peer_tunnel_id 4000 local 192.168.0.2 remote 192.168.0.1 encap udp udp_sport 5000 udp_dport 6000
>     ip l2tp add session name l2tp1 tunnel_id 3000 session_id 1000 peer_session_id 2000
>     ip link set l2tp1 up
>     ip l2tp del tunnel tunnel_id 3000
>     ip l2tp del tunnel tunnel_id 3000
> 
> Fixes: f8ccac0e4493 ("l2tp: put tunnel socket release on a workqueue")
> Reported-by: Jianlin Shi <jishi@redhat.com>
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> ---
> v2: as Tom Parkin explained, we can't remove the tunnel from the
>     per-net list from netlink. v2 uses only a dead flag, and adds
>     corresponding checks during lookups
> 
>  net/l2tp/l2tp_core.c | 18 +++++++++---------
>  net/l2tp/l2tp_core.h |  5 ++++-
>  2 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index ee485df73ccd..3891f0260f2b 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -203,7 +203,8 @@ struct l2tp_tunnel *l2tp_tunnel_get(const struct net *net, u32 tunnel_id)
>  
>  	rcu_read_lock_bh();
>  	list_for_each_entry_rcu(tunnel, &pn->l2tp_tunnel_list, list) {
> -		if (tunnel->tunnel_id == tunnel_id) {
> +		if (tunnel->tunnel_id == tunnel_id &&
> +		    !test_bit(0, &tunnel->dead)) {
>  			l2tp_tunnel_inc_refcount(tunnel);
>  			rcu_read_unlock_bh();
>  
> @@ -390,7 +391,8 @@ struct l2tp_tunnel *l2tp_tunnel_find(const struct net *net, u32 tunnel_id)
>  
>  	rcu_read_lock_bh();
>  	list_for_each_entry_rcu(tunnel, &pn->l2tp_tunnel_list, list) {
> -		if (tunnel->tunnel_id == tunnel_id) {
> +		if (tunnel->tunnel_id == tunnel_id &&
> +		    !test_bit(0, &tunnel->dead)) {
>  			rcu_read_unlock_bh();
>  			return tunnel;
>  		}
> @@ -409,7 +411,7 @@ struct l2tp_tunnel *l2tp_tunnel_find_nth(const struct net *net, int nth)
>  
>  	rcu_read_lock_bh();
>  	list_for_each_entry_rcu(tunnel, &pn->l2tp_tunnel_list, list) {
> -		if (++count > nth) {
> +		if (++count > nth && !test_bit(0, &tunnel->dead)) {
>  			rcu_read_unlock_bh();
>  			return tunnel;
>  		}
> 
I don't get why you're checking the dead flag in l2tp_tunnel_{get,find}*().
Since it can be set concurrently right after test_bit(), it doesn't
protect the caller from getting a tunnel that is being removed by
l2tp_tunnel_delete().
Or have I missed something?

^ permalink raw reply

* Re: [RFC PATCH 1/3] usbnet: Get rid of spammy usbnet "kevent X may have been dropped"
From: Guenter Roeck @ 2017-09-19 16:43 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: Oliver Neukum, Guenter Roeck, Grant Grundler, netdev, linux-usb,
	linux-kernel
In-Reply-To: <20170919161522.995-1-dianders@chromium.org>

On Tue, Sep 19, 2017 at 9:15 AM, Douglas Anderson <dianders@chromium.org> wrote:
> Every once in a while when my system is under a bit of stress I see
> some spammy messages show up in my logs that say:
>
>   kevent X may have been dropped
>
> As far as I can tell these messages aren't terribly useful.  The
> comments around the messages make me think that either workqueues used
> to work differently or that the original author of the code missed a
> sublety related to them.  The error message appears to predate the git
> conversion of the kernel so it's somewhat hard to tell.
>
> Specifically, workqueues should work like this:
>
> A) If a workqueue hasn't been scheduled then schedule_work() schedules
>    it and returns true.
>
> B) If a workqueue has been scheduled (but hasn't started) then
>    schedule_work() will do nothing and return false.
>
> C) If a workqueue has been scheduled (and has started) then
>    schedule_work() will put it on the queue to run again and return
>    true.
>
> Said another way: if you call schedule_work() you can guarantee that
> at least one full runthrough of the work will happen again.  That
> should mean that the work will get processed and I don't see any
> reason to think something should be dropped.
>
> Reading the comments in in usbnet_defer_kevent() made me think that B)
> and C) would be treated the same.  That is: even if we've started the
> work and are 99% of the way through then schedule_work() would return
> false and the work wouldn't be queued again.  If schedule_work()
> really did behave that way then, truly, some amount of work would be
> lost.  ...but it doesn't.
>
> NOTE: if somehow these warnings are useful to mean something then
> perhaps we should change them to make it more obvious.  If it's
> interesting to know when the work is backlogged then we should change
> the spam to say "warning: usbnet is backlogged".
>
> ALSO NOTE: If somehow some of the types of work need to be repeated if
> usbnet_defer_kevent() is called multiple times then that should be
> quite easy to accomplish without dropping any work on the floor.  We
> can just keep an atomic count for that type of work and add a loop
> into usbnet_deferred_kevent().
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Guenter Roeck <groeck@chromium.org>

> ---
>
>  drivers/net/usb/usbnet.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 6510e5cc1817..a3e8dbaadcf9 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -450,19 +450,17 @@ static enum skb_state defer_bh(struct usbnet *dev, struct sk_buff *skb,
>  }
>
>  /* some work can't be done in tasklets, so we use keventd
> - *
> - * NOTE:  annoying asymmetry:  if it's active, schedule_work() fails,
> - * but tasklet_schedule() doesn't.  hope the failure is rare.
>   */
>  void usbnet_defer_kevent (struct usbnet *dev, int work)
>  {
>         set_bit (work, &dev->flags);
> -       if (!schedule_work (&dev->kevent)) {
> -               if (net_ratelimit())
> -                       netdev_err(dev->net, "kevent %d may have been dropped\n", work);
> -       } else {
> -               netdev_dbg(dev->net, "kevent %d scheduled\n", work);
> -       }
> +
> +       /* If work is already started this will mark it to run again when it
> +        * finishes; if we already had work pending and it hadn't started
> +        * yet then that's fine too.
> +        */
> +       schedule_work (&dev->kevent);
> +       netdev_dbg(dev->net, "kevent %d scheduled\n", work);
>  }
>  EXPORT_SYMBOL_GPL(usbnet_defer_kevent);
>
> --
> 2.14.1.690.gbb1197296e-goog
>

^ permalink raw reply

* [PATCH V2 net 7/7] net: hns3: Fixes the premature exit of loop when matching clients
From: Salil Mehta @ 2017-09-19 16:17 UTC (permalink / raw)
  To: davem
  Cc: salil.mehta, yisen.zhuang, lipeng321, mehta.salil.lnk, netdev,
	linux-kernel, linux-rdma, linuxarm
In-Reply-To: <20170919161716.92680-1-salil.mehta@huawei.com>

From: Lipeng <lipeng321@huawei.com>

When register/unregister ae_dev, ae_dev should match all client
in the client_list. Enet and roce can co-exists together so we
should continue checking for enet and roce presence together.
So break should not be there.

Above caused problems in loading and unloading of modules.

Fixes: 38eddd126772 ("net: hns3: Add support of the HNAE3 framework")
Signed-off-by: Lipeng <lipeng321@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hnae3.c | 43 ++++++-----------------------
 1 file changed, 9 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.c b/drivers/net/ethernet/hisilicon/hns3/hnae3.c
index 59efbd6..5bcb223 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hnae3.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.c
@@ -37,20 +37,15 @@ static bool hnae3_client_match(enum hnae3_client_type client_type,
 }
 
 static int hnae3_match_n_instantiate(struct hnae3_client *client,
-				     struct hnae3_ae_dev *ae_dev,
-				     bool is_reg, bool *matched)
+				     struct hnae3_ae_dev *ae_dev, bool is_reg)
 {
 	int ret;
 
-	*matched = false;
-
 	/* check if this client matches the type of ae_dev */
 	if (!(hnae3_client_match(client->type, ae_dev->dev_type) &&
 	      hnae_get_bit(ae_dev->flag, HNAE3_DEV_INITED_B))) {
 		return 0;
 	}
-	/* there is a match of client and dev */
-	*matched = true;
 
 	/* now, (un-)instantiate client by calling lower layer */
 	if (is_reg) {
@@ -69,7 +64,6 @@ int hnae3_register_client(struct hnae3_client *client)
 {
 	struct hnae3_client *client_tmp;
 	struct hnae3_ae_dev *ae_dev;
-	bool matched;
 	int ret = 0;
 
 	mutex_lock(&hnae3_common_lock);
@@ -86,7 +80,7 @@ int hnae3_register_client(struct hnae3_client *client)
 		/* if the client could not be initialized on current port, for
 		 * any error reasons, move on to next available port
 		 */
-		ret = hnae3_match_n_instantiate(client, ae_dev, true, &matched);
+		ret = hnae3_match_n_instantiate(client, ae_dev, true);
 		if (ret)
 			dev_err(&ae_dev->pdev->dev,
 				"match and instantiation failed for port\n");
@@ -102,12 +96,11 @@ EXPORT_SYMBOL(hnae3_register_client);
 void hnae3_unregister_client(struct hnae3_client *client)
 {
 	struct hnae3_ae_dev *ae_dev;
-	bool matched;
 
 	mutex_lock(&hnae3_common_lock);
 	/* un-initialize the client on every matched port */
 	list_for_each_entry(ae_dev, &hnae3_ae_dev_list, node) {
-		hnae3_match_n_instantiate(client, ae_dev, false, &matched);
+		hnae3_match_n_instantiate(client, ae_dev, false);
 	}
 
 	list_del(&client->node);
@@ -124,7 +117,6 @@ int hnae3_register_ae_algo(struct hnae3_ae_algo *ae_algo)
 	const struct pci_device_id *id;
 	struct hnae3_ae_dev *ae_dev;
 	struct hnae3_client *client;
-	bool matched;
 	int ret = 0;
 
 	mutex_lock(&hnae3_common_lock);
@@ -151,13 +143,10 @@ int hnae3_register_ae_algo(struct hnae3_ae_algo *ae_algo)
 		 * initialize the figure out client instance
 		 */
 		list_for_each_entry(client, &hnae3_client_list, node) {
-			ret = hnae3_match_n_instantiate(client, ae_dev, true,
-							&matched);
+			ret = hnae3_match_n_instantiate(client, ae_dev, true);
 			if (ret)
 				dev_err(&ae_dev->pdev->dev,
 					"match and instantiation failed\n");
-			if (matched)
-				break;
 		}
 	}
 
@@ -175,7 +164,6 @@ void hnae3_unregister_ae_algo(struct hnae3_ae_algo *ae_algo)
 	const struct pci_device_id *id;
 	struct hnae3_ae_dev *ae_dev;
 	struct hnae3_client *client;
-	bool matched;
 
 	mutex_lock(&hnae3_common_lock);
 	/* Check if there are matched ae_dev */
@@ -187,12 +175,8 @@ void hnae3_unregister_ae_algo(struct hnae3_ae_algo *ae_algo)
 		/* check the client list for the match with this ae_dev type and
 		 * un-initialize the figure out client instance
 		 */
-		list_for_each_entry(client, &hnae3_client_list, node) {
-			hnae3_match_n_instantiate(client, ae_dev, false,
-						  &matched);
-			if (matched)
-				break;
-		}
+		list_for_each_entry(client, &hnae3_client_list, node)
+			hnae3_match_n_instantiate(client, ae_dev, false);
 
 		ae_algo->ops->uninit_ae_dev(ae_dev);
 		hnae_set_bit(ae_dev->flag, HNAE3_DEV_INITED_B, 0);
@@ -212,7 +196,6 @@ int hnae3_register_ae_dev(struct hnae3_ae_dev *ae_dev)
 	const struct pci_device_id *id;
 	struct hnae3_ae_algo *ae_algo;
 	struct hnae3_client *client;
-	bool matched;
 	int ret = 0;
 
 	mutex_lock(&hnae3_common_lock);
@@ -246,13 +229,10 @@ int hnae3_register_ae_dev(struct hnae3_ae_dev *ae_dev)
 	 * initialize the figure out client instance
 	 */
 	list_for_each_entry(client, &hnae3_client_list, node) {
-		ret = hnae3_match_n_instantiate(client, ae_dev, true,
-						&matched);
+		ret = hnae3_match_n_instantiate(client, ae_dev, true);
 		if (ret)
 			dev_err(&ae_dev->pdev->dev,
 				"match and instantiation failed\n");
-		if (matched)
-			break;
 	}
 
 out_err:
@@ -270,7 +250,6 @@ void hnae3_unregister_ae_dev(struct hnae3_ae_dev *ae_dev)
 	const struct pci_device_id *id;
 	struct hnae3_ae_algo *ae_algo;
 	struct hnae3_client *client;
-	bool matched;
 
 	mutex_lock(&hnae3_common_lock);
 	/* Check if there are matched ae_algo */
@@ -279,12 +258,8 @@ void hnae3_unregister_ae_dev(struct hnae3_ae_dev *ae_dev)
 		if (!id)
 			continue;
 
-		list_for_each_entry(client, &hnae3_client_list, node) {
-			hnae3_match_n_instantiate(client, ae_dev, false,
-						  &matched);
-			if (matched)
-				break;
-		}
+		list_for_each_entry(client, &hnae3_client_list, node)
+			hnae3_match_n_instantiate(client, ae_dev, false);
 
 		ae_algo->ops->uninit_ae_dev(ae_dev);
 		hnae_set_bit(ae_dev->flag, HNAE3_DEV_INITED_B, 0);
-- 
2.7.4

^ permalink raw reply related

* [PATCH V2 net 6/7] net: hns3: Fixes the default VLAN-id of PF
From: Salil Mehta @ 2017-09-19 16:17 UTC (permalink / raw)
  To: davem
  Cc: salil.mehta, yisen.zhuang, lipeng321, mehta.salil.lnk, netdev,
	linux-kernel, linux-rdma, linuxarm, Mingguang Qu
In-Reply-To: <20170919161716.92680-1-salil.mehta@huawei.com>

From: Lipeng <lipeng321@huawei.com>

When there is no vlan id in the packets, hardware will treat the vlan id
as 0 and look for the mac_vlan table. This patch set the default vlan id
of PF as 0. Without this config, it will fail when look for mac_vlan
table, and hardware will drop packets.

Fixes: 6427264ef330 ("net: hns3: Add HNS3 Acceleration Engine &
Compatibility Layer Support")
Signed-off-by: Mingguang Qu <qumingguang@huawei.com>
Signed-off-by: Lipeng <lipeng321@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index 8e172af..74008ef 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -3673,6 +3673,7 @@ static int hclge_init_vlan_config(struct hclge_dev *hdev)
 {
 #define HCLGE_VLAN_TYPE_VF_TABLE   0
 #define HCLGE_VLAN_TYPE_PORT_TABLE 1
+	struct hnae3_handle *handle;
 	int ret;
 
 	ret = hclge_set_vlan_filter_ctrl(hdev, HCLGE_VLAN_TYPE_VF_TABLE,
@@ -3682,8 +3683,11 @@ static int hclge_init_vlan_config(struct hclge_dev *hdev)
 
 	ret = hclge_set_vlan_filter_ctrl(hdev, HCLGE_VLAN_TYPE_PORT_TABLE,
 					 true);
+	if (ret)
+		return ret;
 
-	return ret;
+	handle = &hdev->vport[0].nic;
+	return hclge_set_port_vlan_filter(handle, htons(ETH_P_8021Q), 0, false);
 }
 
 static int hclge_set_mtu(struct hnae3_handle *handle, int new_mtu)
-- 
2.7.4

^ permalink raw reply related

* [PATCH V2 net 5/7] net: hns3: Fixes the ether address copy with appropriate API
From: Salil Mehta @ 2017-09-19 16:17 UTC (permalink / raw)
  To: davem
  Cc: salil.mehta, yisen.zhuang, lipeng321, mehta.salil.lnk, netdev,
	linux-kernel, linux-rdma, linuxarm
In-Reply-To: <20170919161716.92680-1-salil.mehta@huawei.com>

This patch replaces the ethernet address copy instance with more
appropriate ether_addr_copy() function.

Fixes: 6427264ef330 ("net: hns3: Add HNS3 Acceleration Engine &
Compatibility Layer Support")
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index eafd9c6..8e172af 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -1063,8 +1063,7 @@ static int hclge_configure(struct hclge_dev *hdev)
 	hdev->base_tqp_pid = 0;
 	hdev->rss_size_max = 1;
 	hdev->rx_buf_len = cfg.rx_buf_len;
-	for (i = 0; i < ETH_ALEN; i++)
-		hdev->hw.mac.mac_addr[i] = cfg.mac_addr[i];
+	ether_addr_copy(hdev->hw.mac.mac_addr, cfg.mac_addr);
 	hdev->hw.mac.media_type = cfg.media_type;
 	hdev->hw.mac.phy_addr = cfg.phy_addr;
 	hdev->num_desc = cfg.tqp_desc_num;
-- 
2.7.4

^ permalink raw reply related

* [PATCH V2 net 4/7] net: hns3: Fixes the initialization of MAC address in hardware
From: Salil Mehta @ 2017-09-19 16:17 UTC (permalink / raw)
  To: davem
  Cc: salil.mehta, yisen.zhuang, lipeng321, mehta.salil.lnk, netdev,
	linux-kernel, linux-rdma, linuxarm
In-Reply-To: <20170919161716.92680-1-salil.mehta@huawei.com>

From: Lipeng <lipeng321@huawei.com>

This patch fixes the initialization of MAC address, fetched from HNS3
firmware i.e. when it is not randomly generated, to the HNS3 hardware.

Fixes: ca60906d2795 ("net: hns3: Add support of HNS3 Ethernet Driver for
hip08 SoC")
Signed-off-by: Lipeng <lipeng321@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c
index 1c3e294..4d68d6e 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c
@@ -2705,10 +2705,11 @@ static void hns3_init_mac_addr(struct net_device *netdev)
 		eth_hw_addr_random(netdev);
 		dev_warn(priv->dev, "using random MAC address %pM\n",
 			 netdev->dev_addr);
-		/* Also copy this new MAC address into hdev */
-		if (h->ae_algo->ops->set_mac_addr)
-			h->ae_algo->ops->set_mac_addr(h, netdev->dev_addr);
 	}
+
+	if (h->ae_algo->ops->set_mac_addr)
+		h->ae_algo->ops->set_mac_addr(h, netdev->dev_addr);
+
 }
 
 static void hns3_nic_set_priv_ops(struct net_device *netdev)
-- 
2.7.4

^ permalink raw reply related

* [PATCH V2 net 3/7] net: hns3: Fixes ring-to-vector map-and-unmap command
From: Salil Mehta @ 2017-09-19 16:17 UTC (permalink / raw)
  To: davem
  Cc: salil.mehta, yisen.zhuang, lipeng321, mehta.salil.lnk, netdev,
	linux-kernel, linux-rdma, linuxarm, Mingguang Qu
In-Reply-To: <20170919161716.92680-1-salil.mehta@huawei.com>

From: Lipeng <lipeng321@huawei.com>

This patch fixes the vector-to-ring map and unmap command and adds
INT_GL(for, Gap Limiting Interrupts) and VF id to it as required
by the hardware interface.

Fixes: 6427264ef330 ("net: hns3: Add HNS3 Acceleration Engine &
Compatibility Layer Support")
Signed-off-by: Lipeng <lipeng321@huawei.com>
Signed-off-by: Mingguang Qu <qumingguang@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.h  | 8 ++++++--
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 8 ++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.h
index 91ae013..c2b613b 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.h
@@ -238,7 +238,7 @@ struct hclge_tqp_map {
 	u8 rsv[18];
 };
 
-#define HCLGE_VECTOR_ELEMENTS_PER_CMD	11
+#define HCLGE_VECTOR_ELEMENTS_PER_CMD	10
 
 enum hclge_int_type {
 	HCLGE_INT_TX,
@@ -252,8 +252,12 @@ struct hclge_ctrl_vector_chain {
 #define HCLGE_INT_TYPE_S	0
 #define HCLGE_INT_TYPE_M	0x3
 #define HCLGE_TQP_ID_S		2
-#define HCLGE_TQP_ID_M		(0x3fff << HCLGE_TQP_ID_S)
+#define HCLGE_TQP_ID_M		(0x7ff << HCLGE_TQP_ID_S)
+#define HCLGE_INT_GL_IDX_S	13
+#define HCLGE_INT_GL_IDX_M	(0x3 << HCLGE_INT_GL_IDX_S)
 	__le16 tqp_type_and_id[HCLGE_VECTOR_ELEMENTS_PER_CMD];
+	u8 vfid;
+	u8 rsv;
 };
 
 #define HCLGE_TC_NUM		8
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index e324bc6..eafd9c6 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -2680,7 +2680,11 @@ int hclge_map_vport_ring_to_vector(struct hclge_vport *vport, int vector_id,
 			       hnae_get_bit(node->flag, HNAE3_RING_TYPE_B));
 		hnae_set_field(req->tqp_type_and_id[i], HCLGE_TQP_ID_M,
 			       HCLGE_TQP_ID_S,	node->tqp_index);
+		hnae_set_field(req->tqp_type_and_id[i], HCLGE_INT_GL_IDX_M,
+			       HCLGE_INT_GL_IDX_S,
+			       hnae_get_bit(node->flag, HNAE3_RING_TYPE_B));
 		req->tqp_type_and_id[i] = cpu_to_le16(req->tqp_type_and_id[i]);
+		req->vfid = vport->vport_id;
 
 		if (++i >= HCLGE_VECTOR_ELEMENTS_PER_CMD) {
 			req->int_cause_num = HCLGE_VECTOR_ELEMENTS_PER_CMD;
@@ -2764,8 +2768,12 @@ static int hclge_unmap_ring_from_vector(
 			       hnae_get_bit(node->flag, HNAE3_RING_TYPE_B));
 		hnae_set_field(req->tqp_type_and_id[i], HCLGE_TQP_ID_M,
 			       HCLGE_TQP_ID_S,	node->tqp_index);
+		hnae_set_field(req->tqp_type_and_id[i], HCLGE_INT_GL_IDX_M,
+			       HCLGE_INT_GL_IDX_S,
+			       hnae_get_bit(node->flag, HNAE3_RING_TYPE_B));
 
 		req->tqp_type_and_id[i] = cpu_to_le16(req->tqp_type_and_id[i]);
+		req->vfid = vport->vport_id;
 
 		if (++i >= HCLGE_VECTOR_ELEMENTS_PER_CMD) {
 			req->int_cause_num = HCLGE_VECTOR_ELEMENTS_PER_CMD;
-- 
2.7.4

^ permalink raw reply related

* [PATCH V2 net 2/7] net: hns3: Fixes the command used to unmap ring from vector
From: Salil Mehta @ 2017-09-19 16:17 UTC (permalink / raw)
  To: davem
  Cc: salil.mehta, yisen.zhuang, lipeng321, mehta.salil.lnk, netdev,
	linux-kernel, linux-rdma, linuxarm
In-Reply-To: <20170919161716.92680-1-salil.mehta@huawei.com>

From: Lipeng <lipeng321@huawei.com>

This patch fixes the IMP command being used to unmap the vector
from the corresponding ring.

Fixes: 6427264ef330 ("net: hns3: Add HNS3 Acceleration Engine &
Compatibility Layer Support")
Signed-off-by: Lipeng <lipeng321@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index db4e07d..e324bc6 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -2779,7 +2779,7 @@ static int hclge_unmap_ring_from_vector(
 			}
 			i = 0;
 			hclge_cmd_setup_basic_desc(&desc,
-						   HCLGE_OPC_ADD_RING_TO_VECTOR,
+						   HCLGE_OPC_DEL_RING_TO_VECTOR,
 						   false);
 			req->int_vector_id = vector_id;
 		}
-- 
2.7.4

^ permalink raw reply related

* [PATCH V2 net 1/7] net: hns3: Fixes initialization of phy address from firmware
From: Salil Mehta @ 2017-09-19 16:17 UTC (permalink / raw)
  To: davem
  Cc: salil.mehta, yisen.zhuang, lipeng321, mehta.salil.lnk, netdev,
	linux-kernel, linux-rdma, linuxarm
In-Reply-To: <20170919161716.92680-1-salil.mehta@huawei.com>

From: Lipeng <lipeng321@huawei.com>

Default phy address of every port is 0. Therefore, phy address for
each port need to be fetched from firmware and device initialized
with fetched non-default phy address.

Fixes: 6427264ef330 ("net: hns3: Add HNS3 Acceleration Engine &
Compatibility Layer Support")
Signed-off-by: Lipeng <lipeng321@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index bb45365..db4e07d 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -1066,6 +1066,7 @@ static int hclge_configure(struct hclge_dev *hdev)
 	for (i = 0; i < ETH_ALEN; i++)
 		hdev->hw.mac.mac_addr[i] = cfg.mac_addr[i];
 	hdev->hw.mac.media_type = cfg.media_type;
+	hdev->hw.mac.phy_addr = cfg.phy_addr;
 	hdev->num_desc = cfg.tqp_desc_num;
 	hdev->tm_info.num_pg = 1;
 	hdev->tm_info.num_tc = cfg.tc_num;
-- 
2.7.4

^ permalink raw reply related

* [PATCH V2 net 0/7] Bug fixes for the HNS3 Ethernet Driver for Hip08 SoC
From: Salil Mehta @ 2017-09-19 16:17 UTC (permalink / raw)
  To: davem
  Cc: salil.mehta, yisen.zhuang, lipeng321, mehta.salil.lnk, netdev,
	linux-kernel, linux-rdma, linuxarm

This patch set presents some bug fixes for the HNS3 Ethernet driver identified
during internal testing & stabilization efforts.

Change Log:
Patch V2: Resolved comments from Leon Romanovsky
Patch V1: Initial Submit

Lipeng (6):
  net: hns3: Fixes initialization of phy address from firmware
  net: hns3: Fixes the command used to unmap ring from vector
  net: hns3: Fixes ring-to-vector map-and-unmap command
  net: hns3: Fixes the initialization of MAC address in hardware
  net: hns3: Fixes the default VLAN-id of PF
  net: hns3: Fixes the premature exit of loop when matching clients

Salil Mehta (1):
  net: hns3: Fixes the ether address copy with appropriate API

 drivers/net/ethernet/hisilicon/hns3/hnae3.c        | 43 +++++-----------------
 .../net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.h |  8 +++-
 .../ethernet/hisilicon/hns3/hns3pf/hclge_main.c    | 20 ++++++++--
 .../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c |  7 ++--
 4 files changed, 35 insertions(+), 43 deletions(-)

-- 
2.7.4

^ permalink raw reply

* [PATCH net] bpf: do not disable/enable BH in bpf_map_free_id()
From: Eric Dumazet @ 2017-09-19 16:15 UTC (permalink / raw)
  To: David Miller; +Cc: Martin KaFai Lau, Alexei Starovoitov, netdev

From: Eric Dumazet <edumazet@google.com>

syzkaller reported following splat [1]

Since hard irq are disabled by the caller, bpf_map_free_id()
should not try to enable/disable BH.

Another solution would be to change htab_map_delete_elem() to
defer the free_htab_elem() call after
raw_spin_unlock_irqrestore(&b->lock, flags), but this might be not
enough to cover other code paths.

[1]
WARNING: CPU: 1 PID: 8052 at kernel/softirq.c:161 __local_bh_enable_ip
+0x1e/0x160 kernel/softirq.c:161
Kernel panic - not syncing: panic_on_warn set ...

CPU: 1 PID: 8052 Comm: syz-executor1 Not tainted 4.13.0-next-20170915+
#23
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:16 [inline]
 dump_stack+0x194/0x257 lib/dump_stack.c:52
 panic+0x1e4/0x417 kernel/panic.c:181
 __warn+0x1c4/0x1d9 kernel/panic.c:542
 report_bug+0x211/0x2d0 lib/bug.c:183
 fixup_bug+0x40/0x90 arch/x86/kernel/traps.c:178
 do_trap_no_signal arch/x86/kernel/traps.c:212 [inline]
 do_trap+0x260/0x390 arch/x86/kernel/traps.c:261
 do_error_trap+0x120/0x390 arch/x86/kernel/traps.c:298
 do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:311
 invalid_op+0x18/0x20 arch/x86/entry/entry_64.S:905
RIP: 0010:__local_bh_enable_ip+0x1e/0x160 kernel/softirq.c:161
RSP: 0018:ffff8801cdcd7748 EFLAGS: 00010046
RAX: 0000000000000082 RBX: 0000000000000201 RCX: 0000000000000000
RDX: 1ffffffff0b5933c RSI: 0000000000000201 RDI: ffffffff85ac99e0
RBP: ffff8801cdcd7758 R08: ffffffff85b87158 R09: 1ffff10039b9aec6
R10: ffff8801c99f24c0 R11: 0000000000000002 R12: ffffffff817b0b47
R13: dffffc0000000000 R14: ffff8801cdcd77e8 R15: 0000000000000001
 __raw_spin_unlock_bh include/linux/spinlock_api_smp.h:176 [inline]
 _raw_spin_unlock_bh+0x30/0x40 kernel/locking/spinlock.c:207
 spin_unlock_bh include/linux/spinlock.h:361 [inline]
 bpf_map_free_id kernel/bpf/syscall.c:197 [inline]
 __bpf_map_put+0x267/0x320 kernel/bpf/syscall.c:227
 bpf_map_put+0x1a/0x20 kernel/bpf/syscall.c:235
 bpf_map_fd_put_ptr+0x15/0x20 kernel/bpf/map_in_map.c:96
 free_htab_elem+0xc3/0x1b0 kernel/bpf/hashtab.c:658
 htab_map_delete_elem+0x74d/0x970 kernel/bpf/hashtab.c:1063
 map_delete_elem kernel/bpf/syscall.c:633 [inline]
 SYSC_bpf kernel/bpf/syscall.c:1479 [inline]
 SyS_bpf+0x2188/0x46a0 kernel/bpf/syscall.c:1451
 entry_SYSCALL_64_fastpath+0x1f/0xbe

Fixes: f3f1c054c288 ("bpf: Introduce bpf_map ID")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Martin KaFai Lau <kafai@fb.com>
---
 kernel/bpf/syscall.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index cb17e1cd1d434dc2e052a2a9fb0aea967fcf4417..25d074920a009ff682d97bf88e68f466c79bd564 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -186,15 +186,17 @@ static int bpf_map_alloc_id(struct bpf_map *map)
 
 static void bpf_map_free_id(struct bpf_map *map, bool do_idr_lock)
 {
+	unsigned long flags;
+
 	if (do_idr_lock)
-		spin_lock_bh(&map_idr_lock);
+		spin_lock_irqsave(&map_idr_lock, flags);
 	else
 		__acquire(&map_idr_lock);
 
 	idr_remove(&map_idr, map->id);
 
 	if (do_idr_lock)
-		spin_unlock_bh(&map_idr_lock);
+		spin_unlock_irqrestore(&map_idr_lock, flags);
 	else
 		__release(&map_idr_lock);
 }

^ permalink raw reply related

* [RFC PATCH 3/3] usbnet: Fix memory leak when rx_submit() fails
From: Douglas Anderson @ 2017-09-19 16:15 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: groeck, grundler, Douglas Anderson, netdev, linux-usb,
	linux-kernel
In-Reply-To: <20170919161522.995-1-dianders@chromium.org>

If rx_submit() returns an error code then nobody calls usb_free_urb().
That means it's leaked.

NOTE: This problem was found solely by code inspection and not due to
any failing test cases.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/net/usb/usbnet.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index e72547d8d0e6..4c067aaeea5a 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1182,9 +1182,12 @@ usbnet_deferred_kevent (struct work_struct *work)
 					usb_free_urb(urb);
 					goto fail_lowmem;
 				}
-				if (rx_submit (dev, urb, GFP_KERNEL) ==
-				    -ENOLINK)
-					resched = 0;
+				status = rx_submit (dev, urb, GFP_KERNEL);
+				if (status) {
+					usb_free_urb(urb);
+					if (status == -ENOLINK)
+						resched = 0;
+				}
 				usb_autopm_put_interface(dev->intf);
 fail_lowmem:
 				if (resched)
-- 
2.14.1.690.gbb1197296e-goog

^ permalink raw reply related

* [RFC PATCH 2/3] usbnet: Avoid potential races in usbnet_deferred_kevent()
From: Douglas Anderson @ 2017-09-19 16:15 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: groeck, grundler, Douglas Anderson, netdev, linux-usb,
	linux-kernel
In-Reply-To: <20170919161522.995-1-dianders@chromium.org>

In general when you've got a flag communicating that "something needs
to be done" you want to clear that flag _before_ doing the task.  If
you clear the flag _after_ doing the task you end up with the risk
that this will happen:

1. Requester sets flag saying task A needs to be done.
2. Worker comes and stars doing task A.
3. Worker finishes task A but hasn't yet cleared the flag.
4. Requester wants to set flag saying task A needs to be done again.
5. Worker clears the flag without doing anything.

Let's make the usbnet codebase consistently clear the flag _before_ it
does the requested work.  That way if there's another request to do
the work while the work is already in progress it won't be lost.

NOTES:
- No known bugs are fixed by this; it's just found by code inspection.
- This changes the semantics in some of the error conditions.
  -> If we fail to clear the "tx halt" or "rx halt" we still clear the
     flag and thus won't retry the clear next time we happen to be in
     the work function.  Had the old code really wanted to retry these
     events it should have re-scheduled the worker anyway.
  -> If we fail to allocate memory in usb_alloc_urb() we will still
     clear the EVENT_RX_MEMORY flag.  This makes it consistent with
     how we would deal with other failures, including failure to
     allocate a memory chunk in rx_submit().  It can also be noted
     that usb_alloc_urb() in this case is allocating much less than 4K
     worth of data and probably never fails.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/net/usb/usbnet.c | 50 +++++++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 28 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index a3e8dbaadcf9..e72547d8d0e6 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1103,8 +1103,6 @@ static void __handle_link_change(struct usbnet *dev)
 
 	/* hard_mtu or rx_urb_size may change during link change */
 	usbnet_update_max_qlen(dev);
-
-	clear_bit(EVENT_LINK_CHANGE, &dev->flags);
 }
 
 static void usbnet_set_rx_mode(struct net_device *net)
@@ -1118,8 +1116,6 @@ static void __handle_set_rx_mode(struct usbnet *dev)
 {
 	if (dev->driver_info->set_rx_mode)
 		(dev->driver_info->set_rx_mode)(dev);
-
-	clear_bit(EVENT_SET_RX_MODE, &dev->flags);
 }
 
 /* work that cannot be done in interrupt context uses keventd.
@@ -1135,7 +1131,7 @@ usbnet_deferred_kevent (struct work_struct *work)
 	int			status;
 
 	/* usb_clear_halt() needs a thread context */
-	if (test_bit (EVENT_TX_HALT, &dev->flags)) {
+	if (test_and_clear_bit (EVENT_TX_HALT, &dev->flags)) {
 		unlink_urbs (dev, &dev->txq);
 		status = usb_autopm_get_interface(dev->intf);
 		if (status < 0)
@@ -1150,12 +1146,11 @@ usbnet_deferred_kevent (struct work_struct *work)
 				netdev_err(dev->net, "can't clear tx halt, status %d\n",
 					   status);
 		} else {
-			clear_bit (EVENT_TX_HALT, &dev->flags);
 			if (status != -ESHUTDOWN)
 				netif_wake_queue (dev->net);
 		}
 	}
-	if (test_bit (EVENT_RX_HALT, &dev->flags)) {
+	if (test_and_clear_bit (EVENT_RX_HALT, &dev->flags)) {
 		unlink_urbs (dev, &dev->rxq);
 		status = usb_autopm_get_interface(dev->intf);
 		if (status < 0)
@@ -1170,41 +1165,39 @@ usbnet_deferred_kevent (struct work_struct *work)
 				netdev_err(dev->net, "can't clear rx halt, status %d\n",
 					   status);
 		} else {
-			clear_bit (EVENT_RX_HALT, &dev->flags);
 			tasklet_schedule (&dev->bh);
 		}
 	}
 
 	/* tasklet could resubmit itself forever if memory is tight */
-	if (test_bit (EVENT_RX_MEMORY, &dev->flags)) {
+	if (test_and_clear_bit (EVENT_RX_MEMORY, &dev->flags)) {
 		struct urb	*urb = NULL;
 		int resched = 1;
 
-		if (netif_running (dev->net))
+		if (netif_running (dev->net)) {
 			urb = usb_alloc_urb (0, GFP_KERNEL);
-		else
-			clear_bit (EVENT_RX_MEMORY, &dev->flags);
-		if (urb != NULL) {
-			clear_bit (EVENT_RX_MEMORY, &dev->flags);
-			status = usb_autopm_get_interface(dev->intf);
-			if (status < 0) {
-				usb_free_urb(urb);
-				goto fail_lowmem;
-			}
-			if (rx_submit (dev, urb, GFP_KERNEL) == -ENOLINK)
-				resched = 0;
-			usb_autopm_put_interface(dev->intf);
+			if (urb != NULL) {
+				status = usb_autopm_get_interface(dev->intf);
+				if (status < 0) {
+					usb_free_urb(urb);
+					goto fail_lowmem;
+				}
+				if (rx_submit (dev, urb, GFP_KERNEL) ==
+				    -ENOLINK)
+					resched = 0;
+				usb_autopm_put_interface(dev->intf);
 fail_lowmem:
-			if (resched)
-				tasklet_schedule (&dev->bh);
+				if (resched)
+					tasklet_schedule (&dev->bh);
+			}
 		}
+
 	}
 
-	if (test_bit (EVENT_LINK_RESET, &dev->flags)) {
+	if (test_and_clear_bit (EVENT_LINK_RESET, &dev->flags)) {
 		struct driver_info	*info = dev->driver_info;
 		int			retval = 0;
 
-		clear_bit (EVENT_LINK_RESET, &dev->flags);
 		status = usb_autopm_get_interface(dev->intf);
 		if (status < 0)
 			goto skip_reset;
@@ -1221,13 +1214,14 @@ usbnet_deferred_kevent (struct work_struct *work)
 		}
 
 		/* handle link change from link resetting */
+		clear_bit(EVENT_LINK_CHANGE, &dev->flags);
 		__handle_link_change(dev);
 	}
 
-	if (test_bit (EVENT_LINK_CHANGE, &dev->flags))
+	if (test_and_clear_bit (EVENT_LINK_CHANGE, &dev->flags))
 		__handle_link_change(dev);
 
-	if (test_bit (EVENT_SET_RX_MODE, &dev->flags))
+	if (test_and_clear_bit (EVENT_SET_RX_MODE, &dev->flags))
 		__handle_set_rx_mode(dev);
 
 
-- 
2.14.1.690.gbb1197296e-goog

^ permalink raw reply related

* [RFC PATCH 1/3] usbnet: Get rid of spammy usbnet "kevent X may have been dropped"
From: Douglas Anderson @ 2017-09-19 16:15 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: groeck, grundler, Douglas Anderson, netdev, linux-usb,
	linux-kernel

Every once in a while when my system is under a bit of stress I see
some spammy messages show up in my logs that say:

  kevent X may have been dropped

As far as I can tell these messages aren't terribly useful.  The
comments around the messages make me think that either workqueues used
to work differently or that the original author of the code missed a
sublety related to them.  The error message appears to predate the git
conversion of the kernel so it's somewhat hard to tell.

Specifically, workqueues should work like this:

A) If a workqueue hasn't been scheduled then schedule_work() schedules
   it and returns true.

B) If a workqueue has been scheduled (but hasn't started) then
   schedule_work() will do nothing and return false.

C) If a workqueue has been scheduled (and has started) then
   schedule_work() will put it on the queue to run again and return
   true.

Said another way: if you call schedule_work() you can guarantee that
at least one full runthrough of the work will happen again.  That
should mean that the work will get processed and I don't see any
reason to think something should be dropped.

Reading the comments in in usbnet_defer_kevent() made me think that B)
and C) would be treated the same.  That is: even if we've started the
work and are 99% of the way through then schedule_work() would return
false and the work wouldn't be queued again.  If schedule_work()
really did behave that way then, truly, some amount of work would be
lost.  ...but it doesn't.

NOTE: if somehow these warnings are useful to mean something then
perhaps we should change them to make it more obvious.  If it's
interesting to know when the work is backlogged then we should change
the spam to say "warning: usbnet is backlogged".

ALSO NOTE: If somehow some of the types of work need to be repeated if
usbnet_defer_kevent() is called multiple times then that should be
quite easy to accomplish without dropping any work on the floor.  We
can just keep an atomic count for that type of work and add a loop
into usbnet_deferred_kevent().

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/net/usb/usbnet.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 6510e5cc1817..a3e8dbaadcf9 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -450,19 +450,17 @@ static enum skb_state defer_bh(struct usbnet *dev, struct sk_buff *skb,
 }
 
 /* some work can't be done in tasklets, so we use keventd
- *
- * NOTE:  annoying asymmetry:  if it's active, schedule_work() fails,
- * but tasklet_schedule() doesn't.  hope the failure is rare.
  */
 void usbnet_defer_kevent (struct usbnet *dev, int work)
 {
 	set_bit (work, &dev->flags);
-	if (!schedule_work (&dev->kevent)) {
-		if (net_ratelimit())
-			netdev_err(dev->net, "kevent %d may have been dropped\n", work);
-	} else {
-		netdev_dbg(dev->net, "kevent %d scheduled\n", work);
-	}
+
+	/* If work is already started this will mark it to run again when it
+	 * finishes; if we already had work pending and it hadn't started
+	 * yet then that's fine too.
+	 */
+	schedule_work (&dev->kevent);
+	netdev_dbg(dev->net, "kevent %d scheduled\n", work);
 }
 EXPORT_SYMBOL_GPL(usbnet_defer_kevent);
 
-- 
2.14.1.690.gbb1197296e-goog

^ permalink raw reply related

* RE: [PATCH net-next 05/12] net: dsa: b53: Use a macro to define I/O operations
From: David Laight @ 2017-09-19 16:14 UTC (permalink / raw)
  To: 'Florian Fainelli', Vivien Didelot,
	netdev@vger.kernel.org
  Cc: davem@davemloft.net, andrew@lunn.ch
In-Reply-To: <12153E6A-CB96-4EED-91CE-05604B640927@gmail.com>

> >>> +#define b53_build_op(type, op_size, val_type)	\
> >>> +static inline int b53_##type##op_size(struct b53_device *dev, u8
> >page,		\
> >>> +				      u8 reg, val_type val)			\
> >>> +{										\
> >>> +	int ret;								\
> >>> +										\
> >>> +	mutex_lock(&dev->reg_mutex);						\
> >>> +	ret = dev->ops->type##op_size(dev, page, reg, val);			\
> >>> +	mutex_unlock(&dev->reg_mutex);						\
> >>> +										\
> >>> +	return ret;								\
> >>>  }
> >>
> >> Why separate the 'type' and 'op_size' arguments since they
> >> are always pasted together?
> >
> >For read/write48, the value type is u64.
> 
> The way I read David's comment is that instead of calling the macro with read, 48, just combine that
> in a single argument: read48. I don't have a preference about that and can respin eventually.

Indeed, factoring in the type is harder because reads want 'u64 *' not 'u64'.
While that could be factored, it would take more source lines and make
things very obfuscated.

	David


^ permalink raw reply

* Re: [PATCH net-next 3/3] bpf: Test deletion in BPF_MAP_TYPE_LPM_TRIE
From: Daniel Borkmann @ 2017-09-19 16:12 UTC (permalink / raw)
  To: Craig Gallek, Daniel Mack, Alexei Starovoitov, David S . Miller; +Cc: netdev
In-Reply-To: <20170918193057.37644-4-kraigatgoog@gmail.com>

On 09/18/2017 09:30 PM, Craig Gallek wrote:
> From: Craig Gallek <kraig@google.com>
>
> Extend the 'random' operation tests to include a delete operation
> (delete half of the nodes from both lpm implementions and ensure
> that lookups are still equivalent).
>
> Also, add a simple IPv4 test which verifies lookup behavior as nodes
> are deleted from the tree.
>
> Signed-off-by: Craig Gallek <kraig@google.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

^ permalink raw reply

* Re: Re: [PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook
From: Willem de Bruijn @ 2017-09-19 16:12 UTC (permalink / raw)
  To: Nixiaoming
  Cc: xiyou.wangcong@gmail.com, davem@davemloft.net,
	edumazet@google.com, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
In-Reply-To: <CAF=yD-+kG8HHwtL55dRh+mayA+1W0iy7WRRK0uyHfkiX=woH_A@mail.gmail.com>

On Tue, Sep 19, 2017 at 12:09 PM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Tue, Sep 19, 2017 at 3:21 AM, Nixiaoming <nixiaoming@huawei.com> wrote:
>> On Fri, Sep 15, 2017 at 10:46 AM, Willem de Bruijn
>>
>> <willemdebruijn.kernel@gmail.com> wrote:
>>
>>>
>>
>>> In case of failure we also need to unlink and free match. I
>>
>>> sent the following:
>>
>>>
>>
>>> http://patchwork.ozlabs.org/patch/813945/
>>
>>
>>
>> +       spin_lock(&po->bind_lock);
>>
>> +       if (po->running &&
>>
>> +           match->type == type &&
>>
>>            match->prot_hook.type == po->prot_hook.type &&
>>
>>            match->prot_hook.dev == po->prot_hook.dev) {
>>
>>                 err = -ENOSPC;
>>
>> @@ -1761,6 +1760,13 @@  static int fanout_add(struct sock *sk, u16 id, u16
>> type_flags)
>>
>>                           err = 0;
>>
>>                 }
>>
>>        }
>>
>> +       spin_unlock(&po->bind_lock);
>>
>> +
>>
>> +       if (err && !refcount_read(&match->sk_ref)) {
>>
>> +                list_del(&match->list);
>>
>> +                kfree(match);
>>
>> +       }
>>
>>
>>
>>
>>
>> In the function fanout_add add spin_lock to protect po-> running and po->
>> fanout,
>>
>> then whether it should be in the function fanout_release also add spin_lock
>> protection ?
>
> po->bind_lock is held when registering and unregistering the
> protocol hook. fanout_release does access po->running or
> prot_hook.

whoops. does *not* access.

^ permalink raw reply

* Re: [PATCH net-next 2/3] bpf: Add uniqueness invariant to trivial lpm test implementation
From: Daniel Borkmann @ 2017-09-19 16:12 UTC (permalink / raw)
  To: Craig Gallek, Daniel Mack, Alexei Starovoitov, David S . Miller; +Cc: netdev
In-Reply-To: <20170918193057.37644-3-kraigatgoog@gmail.com>

On 09/18/2017 09:30 PM, Craig Gallek wrote:
> From: Craig Gallek <kraig@google.com>
>
> The 'trivial' lpm implementation in this test allows equivalent nodes
> to be added (that is, nodes consisting of the same prefix and prefix
> length).  For lookup operations, this is fine because insertion happens
> at the head of the (singly linked) list and the first, best match is
> returned.  In order to support deletion, the tlpm data structue must
> first enforce uniqueness.  This change modifies the insertion algorithm
> to search for equivalent nodes and remove them.  Note: the
> BPF_MAP_TYPE_LPM_TRIE already has a uniqueness invariant that is
> implemented as node replacement.
>
> Signed-off-by: Craig Gallek <kraig@google.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

^ permalink raw reply

* Re: [PATCH net-next 1/3] bpf: Implement map_delete_elem for BPF_MAP_TYPE_LPM_TRIE
From: Daniel Borkmann @ 2017-09-19 16:12 UTC (permalink / raw)
  To: Craig Gallek, Alexei Starovoitov; +Cc: Daniel Mack, David S . Miller, netdev
In-Reply-To: <CAEfhGixLCqEmR=bwBqC6bT=-j22X5+rSScPb=1qWX3F-mi6H3g@mail.gmail.com>

On 09/19/2017 05:08 PM, Craig Gallek wrote:
> On Mon, Sep 18, 2017 at 6:53 PM, Alexei Starovoitov <ast@fb.com> wrote:
>> On 9/18/17 12:30 PM, Craig Gallek wrote:
[...]
>>> +
>>> +               next_bit = extract_bit(key->data, node->prefixlen);
>>> +               /* If we hit a node that has more than one child or is a
>>> valid
>>> +                * prefix itself, do not remove it. Reset the root of the
>>> trim
>>> +                * path to its descendant on our path.
>>> +                */
>>> +               if (!(node->flags & LPM_TREE_NODE_FLAG_IM) ||
>>> +                   (node->child[0] && node->child[1]))
>>> +                       trim = &node->child[next_bit];
>>> +               node = rcu_dereference_protected(
>>> +                       node->child[next_bit],
>>> lockdep_is_held(&trie->lock));
>>> +       }
>>> +
>>> +       if (!node || node->prefixlen != key->prefixlen ||
>>> +           (node->flags & LPM_TREE_NODE_FLAG_IM)) {
>>> +               ret = -ENOENT;
>>> +               goto out;
>>> +       }
>>> +
>>> +       trie->n_entries--;
>>> +
>>> +       /* If the node we are removing is not a leaf node, simply mark it
>>> +        * as intermediate and we are done.
>>> +        */
>>> +       if (rcu_access_pointer(node->child[0]) ||
>>> +           rcu_access_pointer(node->child[1])) {
>>> +               node->flags |= LPM_TREE_NODE_FLAG_IM;
>>> +               goto out;
>>> +       }
>>> +
>>> +       /* trim should now point to the slot holding the start of a path
>>> from
>>> +        * zero or more intermediate nodes to our leaf node for deletion.
>>> +        */
>>> +       while ((node = rcu_dereference_protected(
>>> +                       *trim, lockdep_is_held(&trie->lock)))) {
>>> +               RCU_INIT_POINTER(*trim, NULL);
>>> +               trim = rcu_access_pointer(node->child[0]) ?
>>> +                       &node->child[0] :
>>> +                       &node->child[1];
>>> +               kfree_rcu(node, rcu);
>>
>> can it be that some of the nodes this loop walks have
>> both child[0] and [1] ?
> No, the loop above will push trim down the walk every time it
> encounters a node with two children.  The only other trim assignment
> is the initial trim = &trie->root.  But the only time we would skip
> the assignment in the loop is if the node being removed is the root.
> If the root had multiple children and is being removed, it would be
> handled by the case that turns the node into an intermediate node
> rather than walking the trim path freeing things.

Looks good to me. We should probably still merge nodes once we turn
a real node into an im which just has a single child attached to it;
parent can be im or real node. Thus, we don't need to traverse this
extra one on lookup.

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

^ permalink raw reply

* Re: Re: [PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook
From: Willem de Bruijn @ 2017-09-19 16:09 UTC (permalink / raw)
  To: Nixiaoming
  Cc: xiyou.wangcong@gmail.com, davem@davemloft.net,
	edumazet@google.com, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
In-Reply-To: <E490CD805F7529488761C40FD9D26EF1299D0CAD@DGGEMA505-MBX.china.huawei.com>

On Tue, Sep 19, 2017 at 3:21 AM, Nixiaoming <nixiaoming@huawei.com> wrote:
> On Fri, Sep 15, 2017 at 10:46 AM, Willem de Bruijn
>
> <willemdebruijn.kernel@gmail.com> wrote:
>
>>
>
>> In case of failure we also need to unlink and free match. I
>
>> sent the following:
>
>>
>
>> http://patchwork.ozlabs.org/patch/813945/
>
>
>
> +       spin_lock(&po->bind_lock);
>
> +       if (po->running &&
>
> +           match->type == type &&
>
>            match->prot_hook.type == po->prot_hook.type &&
>
>            match->prot_hook.dev == po->prot_hook.dev) {
>
>                 err = -ENOSPC;
>
> @@ -1761,6 +1760,13 @@  static int fanout_add(struct sock *sk, u16 id, u16
> type_flags)
>
>                           err = 0;
>
>                 }
>
>        }
>
> +       spin_unlock(&po->bind_lock);
>
> +
>
> +       if (err && !refcount_read(&match->sk_ref)) {
>
> +                list_del(&match->list);
>
> +                kfree(match);
>
> +       }
>
>
>
>
>
> In the function fanout_add add spin_lock to protect po-> running and po->
> fanout,
>
> then whether it should be in the function fanout_release also add spin_lock
> protection ?

po->bind_lock is held when registering and unregistering the
protocol hook. fanout_release does access po->running or
prot_hook.

It is called from packet_release, which does hold the bind_lock
when unregistering the protocol hook.

^ 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