Netdev List
 help / color / mirror / Atom feed
* [net  1/1] tipc: fix failover problem
From: Jon Maloy @ 2018-09-26 19:00 UTC (permalink / raw)
  To: davem, netdev
  Cc: gordan.mihaljevic, tung.q.nguyen, hoang.h.le, jon.maloy,
	canh.d.luu, ying.xue, tipc-discussion

From: LUU Duc Canh <canh.d.luu@dektech.com.au>

We see the following scenario:
1) Link endpoint B on node 1 discovers that its peer endpoint is gone.
   Since there is a second working link, failover procedure is started.
2) Link endpoint A on node 1 sends a FAILOVER message to peer endpoint
   A on node 2. The node item 1->2 goes to state FAILINGOVER.
3) Linke endpoint A/2 receives the failover, and is supposed to take
   down its parallell link endpoint B/2, while producing a FAILOVER
   message to send back to A/1.
4) However, B/2 has already been deleted, so no FAILOVER message can
   created.
5) Node 1->2 remains in state FAILINGOVER forever, refusing to receive
   any messages that can bring B/1 up again. We are left with a non-
   redundant link between node 1 and 2.

We fix this with letting endpoint A/2 build a dummy FAILOVER message
to send to back to A/1, so that the situation can be resolved.

Signed-off-by: LUU Duc Canh <canh.d.luu@dektech.com.au>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
 net/tipc/link.c | 35 +++++++++++++++++++++++++++++++++++
 net/tipc/link.h |  3 +++
 net/tipc/node.c | 11 +++++++++++
 3 files changed, 49 insertions(+)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index 26cc033..4ed650c 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -410,6 +410,11 @@ char *tipc_link_name(struct tipc_link *l)
 	return l->name;
 }
 
+u32 tipc_link_state(struct tipc_link *l)
+{
+	return l->state;
+}
+
 /**
  * tipc_link_create - create a new link
  * @n: pointer to associated node
@@ -1385,6 +1390,36 @@ static void tipc_link_build_proto_msg(struct tipc_link *l, int mtyp, bool probe,
 	__skb_queue_tail(xmitq, skb);
 }
 
+void tipc_link_create_dummy_tnl_msg(struct tipc_link *l,
+				    struct sk_buff_head *xmitq)
+{
+	u32 onode = tipc_own_addr(l->net);
+	struct tipc_msg *hdr, *ihdr;
+	struct sk_buff_head tnlq;
+	struct sk_buff *skb;
+	u32 dnode = l->addr;
+
+	skb_queue_head_init(&tnlq);
+	skb = tipc_msg_create(TUNNEL_PROTOCOL, FAILOVER_MSG,
+			      INT_H_SIZE, BASIC_H_SIZE,
+			      dnode, onode, 0, 0, 0);
+	if (!skb) {
+		pr_warn("%sunable to create tunnel packet\n", link_co_err);
+		return;
+	}
+
+	hdr = buf_msg(skb);
+	msg_set_msgcnt(hdr, 1);
+	msg_set_bearer_id(hdr, l->peer_bearer_id);
+
+	ihdr = (struct tipc_msg *)msg_data(hdr);
+	tipc_msg_init(onode, ihdr, TIPC_LOW_IMPORTANCE, TIPC_DIRECT_MSG,
+		      BASIC_H_SIZE, dnode);
+	msg_set_errcode(ihdr, TIPC_ERR_NO_PORT);
+	__skb_queue_tail(&tnlq, skb);
+	tipc_link_xmit(l, &tnlq, xmitq);
+}
+
 /* tipc_link_tnl_prepare(): prepare and return a list of tunnel packets
  * with contents of the link's transmit and backlog queues.
  */
diff --git a/net/tipc/link.h b/net/tipc/link.h
index 7bc494a3..90488c5 100644
--- a/net/tipc/link.h
+++ b/net/tipc/link.h
@@ -88,6 +88,8 @@ bool tipc_link_bc_create(struct net *net, u32 ownnode, u32 peer,
 			 struct tipc_link **link);
 void tipc_link_tnl_prepare(struct tipc_link *l, struct tipc_link *tnl,
 			   int mtyp, struct sk_buff_head *xmitq);
+void tipc_link_create_dummy_tnl_msg(struct tipc_link *tnl,
+				    struct sk_buff_head *xmitq);
 void tipc_link_build_reset_msg(struct tipc_link *l, struct sk_buff_head *xmitq);
 int tipc_link_fsm_evt(struct tipc_link *l, int evt);
 bool tipc_link_is_up(struct tipc_link *l);
@@ -107,6 +109,7 @@ u16 tipc_link_rcv_nxt(struct tipc_link *l);
 u16 tipc_link_acked(struct tipc_link *l);
 u32 tipc_link_id(struct tipc_link *l);
 char *tipc_link_name(struct tipc_link *l);
+u32 tipc_link_state(struct tipc_link *l);
 char tipc_link_plane(struct tipc_link *l);
 int tipc_link_prio(struct tipc_link *l);
 int tipc_link_window(struct tipc_link *l);
diff --git a/net/tipc/node.c b/net/tipc/node.c
index 68014f1..b0ee25f 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -111,6 +111,7 @@ struct tipc_node {
 	int action_flags;
 	struct list_head list;
 	int state;
+	bool failover_sent;
 	u16 sync_point;
 	int link_cnt;
 	u16 working_links;
@@ -680,6 +681,7 @@ static void __tipc_node_link_up(struct tipc_node *n, int bearer_id,
 		*slot0 = bearer_id;
 		*slot1 = bearer_id;
 		tipc_node_fsm_evt(n, SELF_ESTABL_CONTACT_EVT);
+		n->failover_sent = false;
 		n->action_flags |= TIPC_NOTIFY_NODE_UP;
 		tipc_link_set_active(nl, true);
 		tipc_bcast_add_peer(n->net, nl, xmitq);
@@ -1615,6 +1617,15 @@ static bool tipc_node_check_state(struct tipc_node *n, struct sk_buff *skb,
 			tipc_skb_queue_splice_tail_init(tipc_link_inputq(pl),
 							tipc_link_inputq(l));
 		}
+		/* If parallel link was already down, and this happened before
+		 * the tunnel link came up, FAILOVER was never sent. Ensure that
+		 * FAILOVER is sent to get peer out of NODE_FAILINGOVER state.
+		 */
+		if (n->state != NODE_FAILINGOVER && !n->failover_sent) {
+			tipc_link_create_dummy_tnl_msg(l, xmitq);
+			n->failover_sent = true;
+		}
+
 		/* If pkts arrive out of order, use lowest calculated syncpt */
 		if (less(syncpt, n->sync_point))
 			n->sync_point = syncpt;
-- 
2.1.4

^ permalink raw reply related

* Re: [PATCH net-next v6 23/23] net: WireGuard secure network tunnel
From: Andrew Lunn @ 2018-09-27  1:15 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: linux-kernel, netdev, linux-crypto, davem, gregkh
In-Reply-To: <20180925145622.29959-24-Jason@zx2c4.com>

Hi Jason

I know you have been concentrating on the crypto code, so i'm not
expecting too many changes at the moment in the network code.

It would be good to further reduce the number of checkpatch warnings.
The biggest problem i have at the moment is that all the trivial to
fix warnings are hiding a few more important warnings. There are a few
like these which are not obvious to see:

WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
#2984: FILE: drivers/net/wireguard/noise.c:293:
+       BUG_ON(first_len > BLAKE2S_HASH_SIZE || second_len > BLAKE2S_HASH_SIZE ||

WARNING: Macros with flow control statements should be avoided
#5471: FILE: drivers/net/wireguard/selftest/allowedips.h:456:
+#define init_peer(name) do {                                               \
+               name = kzalloc(sizeof(*name), GFP_KERNEL);                 \
+               if (unlikely(!name)) {                                     \
+                       pr_info("allowedips self-test: out of memory\n");  \
+                       goto free;                                         \
+               }                                                          \
+               kref_init(&name->refcount);                                \
+       } while (0)


The namespace pollution also needs to be addresses. You have some
pretty generic named global symbols. I picked out a few examples from
objdump

00002a94 g     F .text	    00000060 peer_put
00003484 g     F .text	    0000004c timers_stop
00003520 g     F .text	    00000114 packet_queue_init
00002640 g     F .text	    00000034 device_uninit
000026bc g     F .text	    00000288 peer_create
000090d4 g     F .text	    000001bc ratelimiter_init

Please make use of a prefix for global symbols, e.g. wg_.

       Andrew

^ permalink raw reply

* Re: [PATCH RFC,net-next 08/10] flow_dissector: add wake-up-on-lan and queue to flow_action
From: Florian Fainelli @ 2018-09-26 18:51 UTC (permalink / raw)
  To: Pablo Neira Ayuso, netdev
  Cc: davem, thomas.lendacky, ariel.elior, michael.chan, santosh,
	madalin.bucur, yisen.zhuang, salil.mehta, jeffrey.t.kirsher,
	tariqt, saeedm, jiri, idosch, ganeshgr, jakub.kicinski,
	linux-net-drivers, peppe.cavallaro, alexandre.torgue, joabreu,
	grygorii.strashko, andrew, vivien.didelot
In-Reply-To: <20180925192001.2482-9-pablo@netfilter.org>

On 09/25/2018 12:19 PM, Pablo Neira Ayuso wrote:
> These actions need to be added to support bcm sf2 features available
> through the ethtool_rx_flow interface.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply

* Hello
From: Margaret Kwan Wing Han @ 2018-09-26 18:49 UTC (permalink / raw)
  To: Westnet Support

I have a legal business proposal worth $30.5M for you if interested reply 
me for more details. 

Regards, 
Ms Margaret Kwan Wing Han

^ permalink raw reply

* Re: [PATCH RFC,net-next 10/10] dsa: bcm_sf2: use flow_rule infrastructure
From: Florian Fainelli @ 2018-09-26 18:41 UTC (permalink / raw)
  To: Pablo Neira Ayuso, netdev
  Cc: davem, thomas.lendacky, ariel.elior, michael.chan, santosh,
	madalin.bucur, yisen.zhuang, salil.mehta, jeffrey.t.kirsher,
	tariqt, saeedm, jiri, idosch, ganeshgr, jakub.kicinski,
	linux-net-drivers, peppe.cavallaro, alexandre.torgue, joabreu,
	grygorii.strashko, andrew, vivien.didelot
In-Reply-To: <20180925192001.2482-11-pablo@netfilter.org>

Hi Pablo,

On 09/25/2018 12:20 PM, Pablo Neira Ayuso wrote:
> Update this driver to use the flow_rule infrastructure, hence the same
> code to populate hardware IR can be used from ethtool_rx_flow and the
> cls_flower interfaces.

Thanks for doing the conversion, I believe we could change things a
little bit such that there are fewer things to audit for correctness,
see below:

> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  drivers/net/dsa/bcm_sf2_cfp.c | 311 ++++++++++++++++++++++--------------------
>  1 file changed, 166 insertions(+), 145 deletions(-)
> 
> diff --git a/drivers/net/dsa/bcm_sf2_cfp.c b/drivers/net/dsa/bcm_sf2_cfp.c
> index 47c5f272a084..9dace0e25a3a 100644
> --- a/drivers/net/dsa/bcm_sf2_cfp.c
> +++ b/drivers/net/dsa/bcm_sf2_cfp.c
> @@ -251,10 +251,12 @@ static int bcm_sf2_cfp_act_pol_set(struct bcm_sf2_priv *priv,
>  }
>  
>  static void bcm_sf2_cfp_slice_ipv4(struct bcm_sf2_priv *priv,
> -				   struct ethtool_tcpip4_spec *v4_spec,
> +				   struct flow_rule *flow_rule,
>  				   unsigned int slice_num,
>  				   bool mask)
>  {
> +	struct flow_match_ipv4_addrs ipv4;
> +	struct flow_match_ports ports;
>  	u32 reg, offset;
>  
>  	/* C-Tag		[31:24]
> @@ -268,41 +270,54 @@ static void bcm_sf2_cfp_slice_ipv4(struct bcm_sf2_priv *priv,
>  		offset = CORE_CFP_DATA_PORT(4);
>  	core_writel(priv, reg, offset);
>  
> +	flow_rule_match_ipv4_addrs(flow_rule, &ipv4);
> +	flow_rule_match_ports(flow_rule, &ports);
> +
>  	/* UDF_n_A7		[31:24]
>  	 * UDF_n_A6		[23:8]
>  	 * UDF_n_A5		[7:0]
>  	 */
> -	reg = be16_to_cpu(v4_spec->pdst) >> 8;
> -	if (mask)
> +	if (mask) {
> +		reg = be16_to_cpu(ports.mask->dst) >> 8;
>  		offset = CORE_CFP_MASK_PORT(3);
> -	else
> +	} else {
> +		reg = be16_to_cpu(ports.key->dst) >> 8;
>  		offset = CORE_CFP_DATA_PORT(3);
> +	}

For instance here, instead of having to assign "reg" differently, just
have an intermediate struct flow_match_ports variable that either points
to &ports.mask or &ports.key.

I quickly glanced through the changes, and I suspect that they are
correct, but there is unfortunately way too much code movement within
the functions which greatly hinders the ability to have complete
confidence in the changes :)

Can you find a way to only perform the ethtool_rx_flow_spec to flow_rule
conversion as close as possible from the point of use within the callee?

Thanks!
-- 
Florian

^ permalink raw reply

* Re: [PATCH RFC,net-next 00/10] add flow_rule infrastructure
From: Florian Fainelli @ 2018-09-26 18:41 UTC (permalink / raw)
  To: Jakub Kicinski, Pablo Neira Ayuso
  Cc: netdev, davem, thomas.lendacky, ariel.elior, michael.chan,
	santosh, madalin.bucur, yisen.zhuang, salil.mehta,
	jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch, ganeshgr,
	linux-net-drivers, peppe.cavallaro, alexandre.torgue, joabreu,
	grygorii.strashko, andrew, vivien.didelot
In-Reply-To: <20180926085155.73b43531@cakuba.netronome.com>

On 09/26/2018 08:51 AM, Jakub Kicinski wrote:
> On Tue, 25 Sep 2018 21:19:51 +0200, Pablo Neira Ayuso wrote:
>> This patchset is adding a new layer between drivers and the existing
>> software frontends, so it's a bit more code, but it is core
>> infrastructure common to everyone and this comes with benefits for
>> driver developers:
> 
> Thanks a lot for doing this!!

Yeah, this is looks great so far, thanks a lot for tackling this.
-- 
Florian

^ permalink raw reply

* [PATCH] [PATCH net-next] openvswitch: Use correct reply values in datapath and vport ops
From: Yifeng Sun @ 2018-09-26 18:40 UTC (permalink / raw)
  To: netdev; +Cc: Yifeng Sun

This patch fixes the bug that all datapath and vport ops are returning
wrong values (OVS_FLOW_CMD_NEW or OVS_DP_CMD_NEW) in their replies.

Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
---
 net/openvswitch/datapath.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 0f5ce77..6679e96 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1182,14 +1182,14 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
 						       ovs_header->dp_ifindex,
 						       reply, info->snd_portid,
 						       info->snd_seq, 0,
-						       OVS_FLOW_CMD_NEW,
+						       OVS_FLOW_CMD_SET,
 						       ufid_flags);
 			BUG_ON(error < 0);
 		}
 	} else {
 		/* Could not alloc without acts before locking. */
 		reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex,
-						info, OVS_FLOW_CMD_NEW, false,
+						info, OVS_FLOW_CMD_SET, false,
 						ufid_flags);
 
 		if (IS_ERR(reply)) {
@@ -1265,7 +1265,7 @@ static int ovs_flow_cmd_get(struct sk_buff *skb, struct genl_info *info)
 	}
 
 	reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex, info,
-					OVS_FLOW_CMD_NEW, true, ufid_flags);
+					OVS_FLOW_CMD_GET, true, ufid_flags);
 	if (IS_ERR(reply)) {
 		err = PTR_ERR(reply);
 		goto unlock;
@@ -1389,7 +1389,7 @@ static int ovs_flow_cmd_dump(struct sk_buff *skb, struct netlink_callback *cb)
 		if (ovs_flow_cmd_fill_info(flow, ovs_header->dp_ifindex, skb,
 					   NETLINK_CB(cb->skb).portid,
 					   cb->nlh->nlmsg_seq, NLM_F_MULTI,
-					   OVS_FLOW_CMD_NEW, ufid_flags) < 0)
+					   OVS_FLOW_CMD_GET, ufid_flags) < 0)
 			break;
 
 		cb->args[0] = bucket;
@@ -1730,7 +1730,7 @@ static int ovs_dp_cmd_set(struct sk_buff *skb, struct genl_info *info)
 	ovs_dp_change(dp, info->attrs);
 
 	err = ovs_dp_cmd_fill_info(dp, reply, info->snd_portid,
-				   info->snd_seq, 0, OVS_DP_CMD_NEW);
+				   info->snd_seq, 0, OVS_DP_CMD_SET);
 	BUG_ON(err < 0);
 
 	ovs_unlock();
@@ -1761,7 +1761,7 @@ static int ovs_dp_cmd_get(struct sk_buff *skb, struct genl_info *info)
 		goto err_unlock_free;
 	}
 	err = ovs_dp_cmd_fill_info(dp, reply, info->snd_portid,
-				   info->snd_seq, 0, OVS_DP_CMD_NEW);
+				   info->snd_seq, 0, OVS_DP_CMD_GET);
 	BUG_ON(err < 0);
 	ovs_unlock();
 
@@ -1785,7 +1785,7 @@ static int ovs_dp_cmd_dump(struct sk_buff *skb, struct netlink_callback *cb)
 		if (i >= skip &&
 		    ovs_dp_cmd_fill_info(dp, skb, NETLINK_CB(cb->skb).portid,
 					 cb->nlh->nlmsg_seq, NLM_F_MULTI,
-					 OVS_DP_CMD_NEW) < 0)
+					 OVS_DP_CMD_GET) < 0)
 			break;
 		i++;
 	}
@@ -2101,7 +2101,7 @@ static int ovs_vport_cmd_set(struct sk_buff *skb, struct genl_info *info)
 
 	err = ovs_vport_cmd_fill_info(vport, reply, genl_info_net(info),
 				      info->snd_portid, info->snd_seq, 0,
-				      OVS_VPORT_CMD_NEW);
+				      OVS_VPORT_CMD_SET);
 	BUG_ON(err < 0);
 
 	ovs_unlock();
@@ -2182,7 +2182,7 @@ static int ovs_vport_cmd_get(struct sk_buff *skb, struct genl_info *info)
 		goto exit_unlock_free;
 	err = ovs_vport_cmd_fill_info(vport, reply, genl_info_net(info),
 				      info->snd_portid, info->snd_seq, 0,
-				      OVS_VPORT_CMD_NEW);
+				      OVS_VPORT_CMD_GET);
 	BUG_ON(err < 0);
 	rcu_read_unlock();
 
@@ -2218,7 +2218,7 @@ static int ovs_vport_cmd_dump(struct sk_buff *skb, struct netlink_callback *cb)
 						    NETLINK_CB(cb->skb).portid,
 						    cb->nlh->nlmsg_seq,
 						    NLM_F_MULTI,
-						    OVS_VPORT_CMD_NEW) < 0)
+						    OVS_VPORT_CMD_GET) < 0)
 				goto out;
 
 			j++;
-- 
2.7.4

^ permalink raw reply related

* Re: [PATCH net 0/7] net: usb: Check for Wake-on-LAN modes
From: Florian Fainelli @ 2018-09-27  0:15 UTC (permalink / raw)
  To: David Miller
  Cc: netdev, woojung.huh, UNGLinuxDriver, steve.glendinning, keescook,
	akurz, hayeswang, kai.heng.feng, grundler, zhongjiang, bigeasy,
	ran.wang_1, edumazet, linux-usb, linux-kernel
In-Reply-To: <20180926.171218.1890476375628867560.davem@davemloft.net>

On 09/26/2018 05:12 PM, David Miller wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Mon, 24 Sep 2018 13:54:13 -0700
> 
>> Most of our USB Ethernet drivers don't seem to be checking properly
>> whether the user is supplying a correct Wake-on-LAN mode to enter, so
>> the experience as an user could be confusing, since it would generally
>> lead to either no wake-up, or the device not being marked for wake-up.
>>
>> Please review!
> 
> I saw some discussion on patch #3, is there going to be a new version
> of this series or should I apply this one?

I will be sending a v2, please discard this one, thanks!
-- 
Florian

^ permalink raw reply

* Re: [PATCH net 0/7] net: usb: Check for Wake-on-LAN modes
From: David Miller @ 2018-09-27  0:12 UTC (permalink / raw)
  To: f.fainelli
  Cc: netdev, woojung.huh, UNGLinuxDriver, steve.glendinning, keescook,
	akurz, hayeswang, kai.heng.feng, grundler, zhongjiang, bigeasy,
	ran.wang_1, edumazet, linux-usb, linux-kernel
In-Reply-To: <20180924205420.31309-1-f.fainelli@gmail.com>

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Mon, 24 Sep 2018 13:54:13 -0700

> Most of our USB Ethernet drivers don't seem to be checking properly
> whether the user is supplying a correct Wake-on-LAN mode to enter, so
> the experience as an user could be confusing, since it would generally
> lead to either no wake-up, or the device not being marked for wake-up.
> 
> Please review!

I saw some discussion on patch #3, is there going to be a new version
of this series or should I apply this one?

Thanks.

^ permalink raw reply

* Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
From: Jason A. Donenfeld @ 2018-09-27  0:04 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Herbert Xu, Thomas Gleixner, LKML, Netdev,
	Linux Crypto Mailing List, David Miller, Greg Kroah-Hartman,
	Samuel Neves, Andrew Lutomirski, Jean-Philippe Aumasson,
	Russell King - ARM Linux, linux-arm-kernel
In-Reply-To: <CAKv+Gu-WDt8f9qVjLDRPHL7THS0BjtKHThw2RMfHbWBT8Hs8aQ@mail.gmail.com>

On Wed, Sep 26, 2018 at 5:52 PM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> On Wed, 26 Sep 2018 at 17:50, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > On Wed, Sep 26, 2018 at 5:45 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > > So what you have in mind is something like calling simd_relax() every
> > > 4096 bytes or so?
> >
> > That was actually pretty easy, putting together both of your suggestions:
> >
> > static inline bool chacha20_arch(struct chacha20_ctx *state, u8 *dst,
> >                  u8 *src, size_t len,
> >                  simd_context_t *simd_context)
> > {
> >     while (len > PAGE_SIZE) {
> >         chacha20_arch(state, dst, src, PAGE_SIZE, simd_context);
> >         len -= PAGE_SIZE;
> >         src += PAGE_SIZE;
> >         dst += PAGE_SIZE;
> >         simd_relax(simd_context);
> >     }
> >     if (IS_ENABLED(CONFIG_KERNEL_MODE_NEON) && chacha20_use_neon &&
> >         len >= CHACHA20_BLOCK_SIZE * 3 && simd_use(simd_context))
> >         chacha20_neon(dst, src, len, state->key, state->counter);
> >     else
> >         chacha20_arm(dst, src, len, state->key, state->counter);
> >
> >     state->counter[0] += (len + 63) / 64;
> >     return true;
> > }
>
> Nice one :-)
>
> This works for me (but perhaps add a comment as well)

As elegant as my quick recursive solution was, gcc produced kind of
bad code from it, as you might expect. So I've implemented this using
a boring old loop that works the way it's supposed to. This is marked
for v7.

^ permalink raw reply

* Re: [PATCH net 0/2] s390/qeth: fixes 2019-09-26
From: David Miller @ 2018-09-26 17:39 UTC (permalink / raw)
  To: jwi; +Cc: netdev, linux-s390, schwidefsky, heiko.carstens, raspl, ubraun
In-Reply-To: <20180926160710.64739-1-jwi@linux.ibm.com>

From: Julian Wiedmann <jwi@linux.ibm.com>
Date: Wed, 26 Sep 2018 18:07:08 +0200

> please apply two qeth patches for -net. The first is a trivial cleanup
> required for patch #2 by Jean, which fixes a potential endless loop.

Series applied, thank you.

^ permalink raw reply

* Re: [PATCH net-next] net: aquantia: Make function aq_fw1x_set_power() static
From: David Miller @ 2018-09-26 17:32 UTC (permalink / raw)
  To: weiyongjun1
  Cc: igor.russkikh, nikita.danilov, yana.esina, andrew, netdev,
	kernel-janitors
In-Reply-To: <1537964400-179255-1-git-send-email-weiyongjun1@huawei.com>

From: Wei Yongjun <weiyongjun1@huawei.com>
Date: Wed, 26 Sep 2018 12:20:00 +0000

> Fixes the following sparse warning:
> 
> drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c:873:5: warning:
>  symbol 'aq_fw1x_set_power' was not declared. Should it be static?
> 
> Fixes: a0da96c08cfa ("net: aquantia: implement WOL support")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>

Applied.

^ permalink raw reply

* Re: [PATCH net-next] net/tls: Make function get_rec() static
From: David Miller @ 2018-09-26 17:32 UTC (permalink / raw)
  To: weiyongjun1
  Cc: borisp, aviadye, davejwatson, vakul.garg, netdev, kernel-janitors
In-Reply-To: <1537963848-166323-1-git-send-email-weiyongjun1@huawei.com>

From: Wei Yongjun <weiyongjun1@huawei.com>
Date: Wed, 26 Sep 2018 12:10:48 +0000

> Fixes the following sparse warning:
> 
> net/tls/tls_sw.c:655:16: warning:
>  symbol 'get_rec' was not declared. Should it be static?
> 
> Fixes: a42055e8d2c3 ("net/tls: Add support for async encryption of records for performance")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>

Applied.

^ permalink raw reply

* Re: [PATCH net-next] net/core: make function ___gnet_stats_copy_basic() static
From: David Miller @ 2018-09-26 17:31 UTC (permalink / raw)
  To: weiyongjun1; +Cc: john.fastabend, toke, echaudro, netdev, kernel-janitors
In-Reply-To: <1537963785-165570-1-git-send-email-weiyongjun1@huawei.com>

From: Wei Yongjun <weiyongjun1@huawei.com>
Date: Wed, 26 Sep 2018 12:09:45 +0000

> Fixes the following sparse warning:
> 
> net/core/gen_stats.c:166:1: warning:
>  symbol '___gnet_stats_copy_basic' was not declared. Should it be static?
> 
> Fixes: 5e111210a443 ("net/core: Add new basic hardware counter")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>

Applied.

^ permalink raw reply

* Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
From: Andy Lutomirski @ 2018-09-26 17:23 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Ard Biesheuvel, Herbert Xu, Thomas Gleixner, LKML, Netdev,
	Linux Crypto Mailing List, David Miller, Greg Kroah-Hartman,
	Samuel Neves, Andrew Lutomirski, Jean-Philippe Aumasson,
	Russell King - ARM Linux, linux-arm-kernel
In-Reply-To: <CAHmME9pZKUMnnSZ0670rUH4H61g0x88c-B-DZYw_X-8+xgH-=g@mail.gmail.com>



> On Sep 26, 2018, at 10:03 AM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> 
>> On Wed, Sep 26, 2018 at 6:21 PM Andy Lutomirski <luto@amacapital.net> wrote:
>> Are, is what you’re saying that the Zinc chacha20 functions should call simd_relax() every n bytes automatically for some reasonable value of n?  If so, seems sensible, except that some care might be needed to make sure they interact with preemption correctly.
>> 
>> What I mean is: the public Zinc entry points should either be callable in an atomic context or they should not be.  I think this should be checked at runtime in an appropriate place with an __might_sleep or similar.  Or simd_relax should learn to *not* schedule if the result of preempt_enable() leaves it atomic. (And the latter needs to be done in a way that works even on non-preempt kernels, and I don’t remember whether that’s possible.). And this should happen regardless of how many bytes are processed. IOW, calling into Zinc should be equally not atomic-safe for 100 bytes and for 10 MB.
> 
> I'm not sure this is actually a problem. Namely:
> 
> preempt_disable();
> kernel_fpu_begin();
> kernel_fpu_end();
> schedule(); <--- bug!
> 
> Calling kernel_fpu_end() disables preemption, but AFAIK, preemption
> enabling/disabling is recursive, so kernel_fpu_end's use of
> preempt_disable won't actually do anything until the outer preempt
> enable is called:
> 
> preempt_disable();
> kernel_fpu_begin();
> kernel_fpu_end();
> preempt_enable();
> schedule(); <--- works!
> 
> Or am I missing some more subtle point?
> 

No, I think you’re right. I was mid-remembering precisely how simd_relax() worked.

^ permalink raw reply

* Re: [PATCH net-next v3 1/2] netlink: ipv4 igmp join notifications
From: Roopa Prabhu @ 2018-09-26 17:23 UTC (permalink / raw)
  To: Patrick Ruddy
  Cc: David Ahern, netdev, Jiří Pírko, Stephen Hemminger,
	Nikolay Aleksandrov
In-Reply-To: <a3ec4f1c1c6cdde9cfb53a879fe977ce4595a80b.camel@vyatta.att-mail.com>

On Tue, Sep 25, 2018 at 2:34 AM, Patrick Ruddy
<pruddy@vyatta.att-mail.com> wrote:
> On Wed, 2018-09-19 at 21:47 -0700, David Ahern wrote:
>> On 9/18/18 6:12 AM, Patrick Ruddy wrote:
>> >
>> > I've hit a small snag with adding the new groups. The number of defined
>> > groups currently sits at 31 so I can only add one before hitting the
>>
>> I believe you have no more available. RTNLGRP_* has been defined from 0
>> (RTNLGRP_NONE) to 31 (RTNLGRP_IPV6_MROUTE_R) which covers the u32 range.
>>
>> > limit defined by the 32 bit groups bitmask in socakddr_nl. I can use 1
>> > group for both v4 and v6 notifications which seems like the sensible
>> > options since the AF is carried separately, but it breaks the precedent
>> > where there are separate IPV4 and IPV6 groups for IFADDR.
>> >
>> > I have the combined group patches ready and can share them if that's
>> > the preference.
>> >
>> > Has there been any previous discussion about extending the number of
>> > availabel groups?
>> >
>>
>> I have not tried it, but from a prior code review I believe you have you
>> use setsockopt to add groups > 31.
>
> I can certainly join the new groups using setsockopt and
> NETLINK_ADD_MEMBERSHIP.
> I can't see any examples of extending the defined group list within the
> kernel so I assume I just add to the RTNLGRP enum list with a suitable
> comment to indicate that later groups must be joined with the mechanism
> above or am I missing some other way of dynamically adding groups?
>

With a quick look, there are other subsystem specific groups:
xfrm_nlgroups,  nfnetlink_groups ...which i see apps registering using
NETLINK_ADD_MEMBERSHIP.

seems like an overkill to add something like this for your case.

yet another option to consider:
use family: RTNL_FAMILY_IPMR/  RTNL_FAMILY_IP6MR  with RTM_GETADDR/DELADDR
and use the existing groups: RTNLGRP_IPV4_IFADDR /  RTNLGRP_IPV6_IFADDR

(pls check if this will break any existing users)

precedence is ipmr fib rules.

^ permalink raw reply

* Re: [PATCH v3 0/5] netlink: nested policy validation
From: David Miller @ 2018-09-26 17:21 UTC (permalink / raw)
  To: johannes; +Cc: netdev, dsahern
In-Reply-To: <20180926091534.22876-1-johannes@sipsolutions.net>

From: Johannes Berg <johannes@sipsolutions.net>
Date: Wed, 26 Sep 2018 11:15:29 +0200

> This adds nested policy validation, which lets you specify the
> nested attribute type, e.g. NLA_NESTED with sub-policy, or the
> new NLA_NESTED_ARRAY with sub-sub-policy.
> 
> 
> Changes in v2:
>  * move setting the bad attr pointer/message into validate_nla()
>  * remove the recursion patch since that's no longer needed
>  * simply skip the generic bad attr pointer/message setting in
>    case of nested nla_validate() failing since that could fail
>    only due to validate_nla() failing inside, which already sets
>    the extack information
> 
> Changes in v3:
>  * fix NLA_REJECT to have an error message if none is in policy

Looks great Johannes, series applied.

Thanks!

^ permalink raw reply

* Re: [PATCH] net/ncsi: Add NCSI OEM command for FB Tiogapass
From: Vijay Khemka @ 2018-09-26 17:07 UTC (permalink / raw)
  To: Samuel Mendoza-Jonas, Joel Stanley
  Cc: linux-aspeed@lists.ozlabs.org, OpenBMC Maillist, Sai Dasari,
	Amithash Prasad, netdev@vger.kernel.org, Justin.Lee1@Dell.com
In-Reply-To: <7cd8a49a057f1032e126b1e104fcf61e4956e06d.camel@mendozajonas.com>

>    Hi Vijay,
    
 >  Thanks for the patch; before I get too into a review though I'd like to
 >  loop in Justin (cc'd) who I know is also working on an OEM command patch.
 >  The changes here are very specific (eg. a command specific config option
 >  "CONFIG_NCSI_OEM_CMD_GET_MAC"), which is ok on a small scale but if we
 >  start to add an increasing amount of commands could get out of hand.
 >  As I understand Justin's version adds a generic handler, using the NCSI
 >  Netlink interface to pass OEM commands and responses to and from
 >  userspace, which does the actual packet handling.
 >  It would be good to compare these two approaches first before committing
 >  to any one path
    
Hi Sam,
My oem command handler is generic and can be used for any oem commands and oem response handler can be made more generic. We can certainly write a wrapper to support netlink oem command to receive form user space. There are Mellanox specific functions which sends Mellanox specific request. These needed as a part of initial configuration. We can remove Kconfig option with more generic approach.

-Vijay 


^ permalink raw reply

* RE: [PATCH] hv_netvsc: Make sure out channel is fully opened on send
From: Haiyang Zhang @ 2018-09-26 17:13 UTC (permalink / raw)
  To: Mohammed Gamal, Stephen Hemminger, netdev@vger.kernel.org
  Cc: KY Srinivasan, vkuznets, otubo@redhat.com, cavery,
	linux-kernel@vger.kernel.org, devel@linuxdriverproject.org
In-Reply-To: <1537979659-26979-1-git-send-email-mgamal@redhat.com>



> -----Original Message-----
> From: Mohammed Gamal <mgamal@redhat.com>
> Sent: Wednesday, September 26, 2018 12:34 PM
> To: Stephen Hemminger <sthemmin@microsoft.com>; netdev@vger.kernel.org
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; vkuznets <vkuznets@redhat.com>;
> otubo@redhat.com; cavery <cavery@redhat.com>; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org; Mohammed Gamal
> <mgamal@redhat.com>
> Subject: [PATCH] hv_netvsc: Make sure out channel is fully opened on send
> 
> Dring high network traffic changes to network interface parameters such as
> number of channels or MTU can cause a kernel panic with a NULL pointer
> dereference. This is due to netvsc_device_remove() being called and
> deallocating the channel ring buffers, which can then be accessed by
> netvsc_send_pkt() before they're allocated on calling
> netvsc_device_add()
> 
> The patch fixes this problem by checking the channel state and returning
> ENODEV if not yet opened. We also move the call to hv_ringbuf_avail_percent()
> which may access the uninitialized ring buffer.
> 
> Signed-off-by: Mohammed Gamal <mgamal@redhat.com>
> ---
>  drivers/net/hyperv/netvsc.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c index
> fe01e14..75f1b31 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -825,7 +825,12 @@ static inline int netvsc_send_pkt(
>  	struct netdev_queue *txq = netdev_get_tx_queue(ndev, packet->q_idx);
>  	u64 req_id;
>  	int ret;
> -	u32 ring_avail = hv_get_avail_to_write_percent(&out_channel-
> >outbound);
> +	u32 ring_avail;
> +
> +	if (out_channel->state != CHANNEL_OPENED_STATE)
> +		return -ENODEV;
> +
> +	ring_avail = hv_get_avail_to_write_percent(&out_channel->outbound);

When you reproducing the NULL ptr panic, does your kernel include the following patch?
hv_netvsc: common detach logic
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=7b2ee50c0cd513a176a26a71f2989facdd75bfea

We call netif_tx_disable(ndev) and netif_device_detach(ndev) before doing the changes 
on MTU or #channels. So there should be no call to start_xmit() when channel is not ready.

If you see the check for CHANNEL_OPENED_STATE is still necessary on upstream kernel (including 
the patch " common detach logic "), we should debug further on the code and find out the 
root cause.

Thanks,
- Haiyang

^ permalink raw reply

* Re: [PATCH net-next v2 0/9] net: bridge: convert bool options to bits
From: David Miller @ 2018-09-26 17:04 UTC (permalink / raw)
  To: nikolay; +Cc: netdev, roopa, stephen, bridge, idosch
In-Reply-To: <2ab2aeb3-7667-816c-a1cd-97dbe723db33@cumulusnetworks.com>

From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Date: Wed, 26 Sep 2018 17:04:03 +0300

> On 26/09/18 17:00, Nikolay Aleksandrov wrote:
>> Hi,
>> A lot of boolean bridge options have been added around the net_bridge
>> structure resulting in holes and more importantly different cache lines
>> that need to be fetched in the fast path. This set moves all of those
>> to bits in a bitfield which resides in a hot cache line thus reducing
>> the size of net_bridge, the number of holes and the number of cache
>> lines needed for the fast path.
>> The set is also sent in preparation for new boolean options to avoid
>> spreading them in the structure and making new holes.
>> One nice side-effect is that we avoid potential race conditions by using
>> the bitops since some of the options were bits being directly set in
>> parallel risking hard to debug issues (has_ipv6_addr).
>> 
>> Before:
>>  size: 1184, holes: 8, sum holes: 30
>> After:
>>  size: 1160, holes: 3, sum holes: 7
>> 
>> Patch 01 is a trivial style fix
>> Patch 02 adds the new options bitfield and converts the vlan boolean
>>          options to bits
>> Patches 03-08 convert the rest of the boolean options to bits
>> Patch 09 re-arranges a few fields in net_bridge to further reduce size
>> 
>> v2: patch 09: remove the comment about offload_fwd_mark in net_bridge and
>>     leave it in the last 4 bytes, thanks to Ido for spotting it
> 
> And obviously it's not in the last 4 bytes, but that's fine. That
> must've said just: "leave it where it is now".

I fixed up the wording and applied this series, looks great!

Thanks!

^ permalink raw reply

* Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
From: Jason A. Donenfeld @ 2018-09-26 17:03 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Ard Biesheuvel, Herbert Xu, Thomas Gleixner, LKML, Netdev,
	Linux Crypto Mailing List, David Miller, Greg Kroah-Hartman,
	Samuel Neves, Andrew Lutomirski, Jean-Philippe Aumasson,
	Russell King - ARM Linux, linux-arm-kernel
In-Reply-To: <D93C2C15-DC70-4A95-9350-AD4036C9556A@amacapital.net>

On Wed, Sep 26, 2018 at 6:21 PM Andy Lutomirski <luto@amacapital.net> wrote:
> Are, is what you’re saying that the Zinc chacha20 functions should call simd_relax() every n bytes automatically for some reasonable value of n?  If so, seems sensible, except that some care might be needed to make sure they interact with preemption correctly.
>
> What I mean is: the public Zinc entry points should either be callable in an atomic context or they should not be.  I think this should be checked at runtime in an appropriate place with an __might_sleep or similar.  Or simd_relax should learn to *not* schedule if the result of preempt_enable() leaves it atomic. (And the latter needs to be done in a way that works even on non-preempt kernels, and I don’t remember whether that’s possible.). And this should happen regardless of how many bytes are processed. IOW, calling into Zinc should be equally not atomic-safe for 100 bytes and for 10 MB.

I'm not sure this is actually a problem. Namely:

preempt_disable();
kernel_fpu_begin();
kernel_fpu_end();
schedule(); <--- bug!

Calling kernel_fpu_end() disables preemption, but AFAIK, preemption
enabling/disabling is recursive, so kernel_fpu_end's use of
preempt_disable won't actually do anything until the outer preempt
enable is called:

preempt_disable();
kernel_fpu_begin();
kernel_fpu_end();
preempt_enable();
schedule(); <--- works!

Or am I missing some more subtle point?

Jason

^ permalink raw reply

* Re: [PATCH net-next 00/15] s390/net: updates 2018-09-26
From: David Miller @ 2018-09-26 17:02 UTC (permalink / raw)
  To: jwi; +Cc: netdev, linux-s390, schwidefsky, heiko.carstens, raspl, ubraun
In-Reply-To: <20180926162916.102720-1-jwi@linux.ibm.com>

From: Julian Wiedmann <jwi@linux.ibm.com>
Date: Wed, 26 Sep 2018 18:29:01 +0200

> please apply one more series of cleanups and small improvements for qeth
> to net-next. Note that one patch needs to touch both af_iucv and qeth, in
> order to untangle their receive paths.

Series applied, thank you.

^ permalink raw reply

* Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
From: Ard Biesheuvel @ 2018-09-26 16:54 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Herbert Xu, Thomas Gleixner, Linux Kernel Mailing List,
	<netdev@vger.kernel.org>,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE, David S. Miller,
	Greg Kroah-Hartman, Samuel Neves, Andy Lutomirski,
	Jean-Philippe Aumasson, Russell King, linux-arm-kernel
In-Reply-To: <CAHmME9p5b=L0FSL72gCszhvut-kr=aD4ZniY9qsJxiBnZk8qNQ@mail.gmail.com>

On Wed, 26 Sep 2018 at 17:41, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> On Wed, Sep 26, 2018 at 4:02 PM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> > I don't think it makes sense to keep
> > it simple now and add the complexity later (and the same concern
> > applies to async support btw).
>
> Ugh, no. I don't want to add needless complexity, period. Zinc is
> synchronous, not asynchronous. It provides software implementations.
> That's what it does. While many of your reviews have been useful, many
> of your comments indicate some desire to change and mold the purpose
> and focus of Zinc away from Zinc's intents. Stop that. It's not going
> to become a bloated mess of "things Ard wanted and quipped about on
> LKML." Things like these only serve to filibuster the patchset
> indefinitely. But maybe that's what you'd like all along? Hard to
> tell, honestly. So, no, sorry, Zinc isn't gaining an async interface
> right now.

Framing it as /needless/ complexity does not help at all. The changes
you are proposing are very useful, but nobody wants two crypto
subsystems with two different maintainers in the kernel, so I would
like to understand where this is going in the future. I am not saying
it should block these patches though.

Also, I have spent a *lot* of time looking at your code, and trying to
make it better, especially for use cases that weren't on your radar to
begin with (e.g., 'pet projects' [your words] like the Cortex-A7 which
will be in almost every new 32-bit Android phone). So characterizing
my feedback as some kind of sabotage is not very productive either.

Contrary to what you seem to think, I am not deeply invested in the
crypto API. What I do care about is that the ARM crypto pieces in the
kernel are maintained, supported and improved by someone who
understands the use cases Linaro's members care about, and is willing
to make an effort to gain such understanding if he doesn't. I have no
doubt that your involvement in the kernel's crypto subsystem will have
a significant positive impact when it comes to code quality,
robustness and usability. I'd just like to see a bit more
consideration for other aspects of kernel programming, e.g.,
preemption under -rt, stack size constraints, coding style, importing
code from other projects etc. - please try to be less dismissive of
feedback first time around, but try to understand why people are
raising these issues; I'm sure you will appreciate it when future
contributors to zinc will do the same.

^ permalink raw reply

* [PATCH net-next 14/15] s390/qeth: clean up drop conditions for received cmds
From: Julian Wiedmann @ 2018-09-26 16:29 UTC (permalink / raw)
  To: David Miller
  Cc: netdev, linux-s390, Martin Schwidefsky, Heiko Carstens,
	Stefan Raspl, Ursula Braun, Julian Wiedmann
In-Reply-To: <20180926162916.102720-1-jwi@linux.ibm.com>

If qeth_check_ipa_data() consumed an event, there's no point in
processing it further. So drop it early, and make the surrounding code
a tiny bit more readable.

Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
---
 drivers/s390/net/qeth_core_main.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
index 954dc6c688e8..9cbdc6760aba 100644
--- a/drivers/s390/net/qeth_core_main.c
+++ b/drivers/s390/net/qeth_core_main.c
@@ -826,16 +826,17 @@ static void qeth_send_control_data_cb(struct qeth_card *card,
 	if (IS_IPA(iob->data)) {
 		cmd = (struct qeth_ipa_cmd *) PDU_ENCAPSULATION(iob->data);
 		cmd = qeth_check_ipa_data(card, cmd);
-	}
-	if ((cmd == NULL) && (card->state != CARD_STATE_DOWN))
-		goto out;
-	/*in case of OSN : check if cmd is set */
-	if (card->info.type == QETH_CARD_TYPE_OSN &&
-	    cmd &&
-	    cmd->hdr.command != IPA_CMD_STARTLAN &&
-	    card->osn_info.assist_cb != NULL) {
-		card->osn_info.assist_cb(card->dev, cmd);
-		goto out;
+		if (!cmd)
+			goto out;
+		if (IS_OSN(card) && card->osn_info.assist_cb &&
+		    cmd->hdr.command != IPA_CMD_STARTLAN) {
+			card->osn_info.assist_cb(card->dev, cmd);
+			goto out;
+		}
+	} else {
+		/* non-IPA commands should only flow during initialization */
+		if (card->state != CARD_STATE_DOWN)
+			goto out;
 	}
 
 	spin_lock_irqsave(&card->lock, flags);
-- 
2.16.4

^ permalink raw reply related

* [PATCH net-next 10/15] s390/qeth: remove CARD_FROM_CDEV helper
From: Julian Wiedmann @ 2018-09-26 16:29 UTC (permalink / raw)
  To: David Miller
  Cc: netdev, linux-s390, Martin Schwidefsky, Heiko Carstens,
	Stefan Raspl, Ursula Braun, Julian Wiedmann
In-Reply-To: <20180926162916.102720-1-jwi@linux.ibm.com>

The cdev-to-card translation walks through two layers of drvdata,
with no locking or refcounting (where eg. the ccwgroup core only
accesses a cdev's drvdata while holding the ccwlock).

This might be safe for now, but any careless usage of the helper has the
potential for subtle races and use-after-free's. Luckily there's only
one occurrence where we _really_ need it (in qeth_irq()), for any other
user we can just pass through an appropriate card pointer.

Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
---
 drivers/s390/net/qeth_core_main.c | 73 ++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 44 deletions(-)

diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
index 86b9cce1f483..caa5d109841c 100644
--- a/drivers/s390/net/qeth_core_main.c
+++ b/drivers/s390/net/qeth_core_main.c
@@ -746,18 +746,10 @@ static int qeth_check_idx_response(struct qeth_card *card,
 	return 0;
 }
 
-static struct qeth_card *CARD_FROM_CDEV(struct ccw_device *cdev)
-{
-	struct qeth_card *card = dev_get_drvdata(&((struct ccwgroup_device *)
-		dev_get_drvdata(&cdev->dev))->dev);
-	return card;
-}
-
 static struct qeth_cmd_buffer *__qeth_get_buffer(struct qeth_channel *channel)
 {
 	__u8 index;
 
-	QETH_CARD_TEXT(CARD_FROM_CDEV(channel->ccwdev), 6, "getbuff");
 	index = channel->io_buf_no;
 	do {
 		if (channel->iob[index].state == BUF_STATE_FREE) {
@@ -778,7 +770,6 @@ void qeth_release_buffer(struct qeth_channel *channel,
 {
 	unsigned long flags;
 
-	QETH_CARD_TEXT(CARD_FROM_CDEV(channel->ccwdev), 6, "relbuff");
 	spin_lock_irqsave(&channel->iob_lock, flags);
 	iob->state = BUF_STATE_FREE;
 	iob->callback = qeth_send_control_data_cb;
@@ -980,16 +971,15 @@ void qeth_schedule_recovery(struct qeth_card *card)
 }
 EXPORT_SYMBOL_GPL(qeth_schedule_recovery);
 
-static int qeth_get_problem(struct ccw_device *cdev, struct irb *irb)
+static int qeth_get_problem(struct qeth_card *card, struct ccw_device *cdev,
+			    struct irb *irb)
 {
 	int dstat, cstat;
 	char *sense;
-	struct qeth_card *card;
 
 	sense = (char *) irb->ecw;
 	cstat = irb->scsw.cmd.cstat;
 	dstat = irb->scsw.cmd.dstat;
-	card = CARD_FROM_CDEV(cdev);
 
 	if (cstat & (SCHN_STAT_CHN_CTRL_CHK | SCHN_STAT_INTF_CTRL_CHK |
 		     SCHN_STAT_CHN_DATA_CHK | SCHN_STAT_CHAIN_CHECK |
@@ -1029,14 +1019,11 @@ static int qeth_get_problem(struct ccw_device *cdev, struct irb *irb)
 	return 0;
 }
 
-static long __qeth_check_irb_error(struct ccw_device *cdev,
-		unsigned long intparm, struct irb *irb)
+static long qeth_check_irb_error(struct qeth_card *card,
+				 struct ccw_device *cdev, unsigned long intparm,
+				 struct irb *irb)
 {
-	struct qeth_card *card;
-
-	card = CARD_FROM_CDEV(cdev);
-
-	if (!card || !IS_ERR(irb))
+	if (!IS_ERR(irb))
 		return 0;
 
 	switch (PTR_ERR(irb)) {
@@ -1073,10 +1060,13 @@ static void qeth_irq(struct ccw_device *cdev, unsigned long intparm,
 	int rc;
 	int cstat, dstat;
 	struct qeth_cmd_buffer *iob = NULL;
+	struct ccwgroup_device *gdev;
 	struct qeth_channel *channel;
 	struct qeth_card *card;
 
-	card = CARD_FROM_CDEV(cdev);
+	/* while we hold the ccwdev lock, this stays valid: */
+	gdev = dev_get_drvdata(&cdev->dev);
+	card = dev_get_drvdata(&gdev->dev);
 	if (!card)
 		return;
 
@@ -1096,7 +1086,7 @@ static void qeth_irq(struct ccw_device *cdev, unsigned long intparm,
 	if (qeth_intparm_is_iob(intparm))
 		iob = (struct qeth_cmd_buffer *) __va((addr_t)intparm);
 
-	if (__qeth_check_irb_error(cdev, intparm, irb)) {
+	if (qeth_check_irb_error(card, cdev, intparm, irb)) {
 		/* IO was terminated, free its resources. */
 		if (iob)
 			qeth_release_buffer(iob->channel, iob);
@@ -1151,7 +1141,7 @@ static void qeth_irq(struct ccw_device *cdev, unsigned long intparm,
 			channel->state = CH_STATE_DOWN;
 			goto out;
 		}
-		rc = qeth_get_problem(cdev, irb);
+		rc = qeth_get_problem(card, cdev, irb);
 		if (rc) {
 			card->read_or_write_problem = 1;
 			qeth_clear_ipacmd_list(card);
@@ -1514,12 +1504,11 @@ static struct qeth_card *qeth_alloc_card(struct ccwgroup_device *gdev)
 	return NULL;
 }
 
-static int qeth_clear_channel(struct qeth_channel *channel)
+static int qeth_clear_channel(struct qeth_card *card,
+			      struct qeth_channel *channel)
 {
-	struct qeth_card *card;
 	int rc;
 
-	card = CARD_FROM_CDEV(channel->ccwdev);
 	QETH_CARD_TEXT(card, 3, "clearch");
 	spin_lock_irq(get_ccwdev_lock(channel->ccwdev));
 	rc = ccw_device_clear(channel->ccwdev, QETH_CLEAR_CHANNEL_PARM);
@@ -1537,12 +1526,11 @@ static int qeth_clear_channel(struct qeth_channel *channel)
 	return 0;
 }
 
-static int qeth_halt_channel(struct qeth_channel *channel)
+static int qeth_halt_channel(struct qeth_card *card,
+			     struct qeth_channel *channel)
 {
-	struct qeth_card *card;
 	int rc;
 
-	card = CARD_FROM_CDEV(channel->ccwdev);
 	QETH_CARD_TEXT(card, 3, "haltch");
 	spin_lock_irq(get_ccwdev_lock(channel->ccwdev));
 	rc = ccw_device_halt(channel->ccwdev, QETH_HALT_CHANNEL_PARM);
@@ -1564,9 +1552,9 @@ static int qeth_halt_channels(struct qeth_card *card)
 	int rc1 = 0, rc2 = 0, rc3 = 0;
 
 	QETH_CARD_TEXT(card, 3, "haltchs");
-	rc1 = qeth_halt_channel(&card->read);
-	rc2 = qeth_halt_channel(&card->write);
-	rc3 = qeth_halt_channel(&card->data);
+	rc1 = qeth_halt_channel(card, &card->read);
+	rc2 = qeth_halt_channel(card, &card->write);
+	rc3 = qeth_halt_channel(card, &card->data);
 	if (rc1)
 		return rc1;
 	if (rc2)
@@ -1579,9 +1567,9 @@ static int qeth_clear_channels(struct qeth_card *card)
 	int rc1 = 0, rc2 = 0, rc3 = 0;
 
 	QETH_CARD_TEXT(card, 3, "clearchs");
-	rc1 = qeth_clear_channel(&card->read);
-	rc2 = qeth_clear_channel(&card->write);
-	rc3 = qeth_clear_channel(&card->data);
+	rc1 = qeth_clear_channel(card, &card->read);
+	rc2 = qeth_clear_channel(card, &card->write);
+	rc3 = qeth_clear_channel(card, &card->data);
 	if (rc1)
 		return rc1;
 	if (rc2)
@@ -1810,17 +1798,16 @@ static void qeth_init_func_level(struct qeth_card *card)
 	}
 }
 
-static int qeth_idx_activate_get_answer(struct qeth_channel *channel,
+static int qeth_idx_activate_get_answer(struct qeth_card *card,
+					struct qeth_channel *channel,
 					void (*reply_cb)(struct qeth_card *,
 							 struct qeth_channel *,
 							 struct qeth_cmd_buffer *))
 {
 	struct qeth_cmd_buffer *iob;
 	int rc;
-	struct qeth_card *card;
 
 	QETH_DBF_TEXT(SETUP, 2, "idxanswr");
-	card = CARD_FROM_CDEV(channel->ccwdev);
 	iob = qeth_get_buffer(channel);
 	if (!iob)
 		return -ENOMEM;
@@ -1854,20 +1841,18 @@ static int qeth_idx_activate_get_answer(struct qeth_channel *channel,
 	return rc;
 }
 
-static int qeth_idx_activate_channel(struct qeth_channel *channel,
+static int qeth_idx_activate_channel(struct qeth_card *card,
+				     struct qeth_channel *channel,
 				     void (*reply_cb)(struct qeth_card *,
 						      struct qeth_channel *,
 						      struct qeth_cmd_buffer *))
 {
-	struct qeth_card *card;
 	struct qeth_cmd_buffer *iob;
 	__u16 temp;
 	__u8 tmp;
 	int rc;
 	struct ccw_dev_id temp_devid;
 
-	card = CARD_FROM_CDEV(channel->ccwdev);
-
 	QETH_DBF_TEXT(SETUP, 2, "idxactch");
 
 	iob = qeth_get_buffer(channel);
@@ -1925,7 +1910,7 @@ static int qeth_idx_activate_channel(struct qeth_channel *channel,
 		QETH_DBF_TEXT_(SETUP, 2, "2err%d", -ETIME);
 		return -ETIME;
 	}
-	return qeth_idx_activate_get_answer(channel, reply_cb);
+	return qeth_idx_activate_get_answer(card, channel, reply_cb);
 }
 
 static int qeth_peer_func_level(int level)
@@ -5131,7 +5116,7 @@ int qeth_core_hardsetup_card(struct qeth_card *card)
 	qeth_determine_capabilities(card);
 	qeth_init_tokens(card);
 	qeth_init_func_level(card);
-	rc = qeth_idx_activate_channel(&card->read, qeth_idx_read_cb);
+	rc = qeth_idx_activate_channel(card, &card->read, qeth_idx_read_cb);
 	if (rc == -ERESTARTSYS) {
 		QETH_DBF_TEXT(SETUP, 2, "break2");
 		return rc;
@@ -5142,7 +5127,7 @@ int qeth_core_hardsetup_card(struct qeth_card *card)
 		else
 			goto retry;
 	}
-	rc = qeth_idx_activate_channel(&card->write, qeth_idx_write_cb);
+	rc = qeth_idx_activate_channel(card, &card->write, qeth_idx_write_cb);
 	if (rc == -ERESTARTSYS) {
 		QETH_DBF_TEXT(SETUP, 2, "break3");
 		return rc;
-- 
2.16.4

^ permalink raw reply related


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