All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings
@ 2020-07-21 17:31 Tom Parkin
  2020-07-21 17:31 ` [PATCH 01/29] l2tp: cleanup whitespace around operators Tom Parkin
                   ` (29 more replies)
  0 siblings, 30 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:31 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

l2tp hasn't been kept up to date with the static analysis checks offered
by checkpatch.pl.

This patchset makes l2tp checkpatch-clean, with some exceptions as
described below.

The majority of these issues are trivial: for example small whitespace
tweaks, comment style, line breaks and indentation.  The first patches
in the series deal with these.

The more substantive patches fall into two categories:

 * Some rearrangement of code to better deal with CONFIG_IPV6 ifdefs,
   thereby avoiding checkpatch warnings.

 * Removal of BUG_ON in l2tp.  Some uses of BUG_ON offered little or no
   benefit in the current codebase: these have been removed.  Others do
   help to flag an unexpected condition; and these have been replaced
   with a WARN_ON and appropriate recovery code.

The following checkpatch warnings are not addressed:

 * Two line length warnings for lines of 103 and 101 characters.  These
   lines are more readable without a linebreak so I prefer to leave them
   as-is.

 * Two warnings about unbalanced braces around an else statement in
   l2tp_core.c.  These result from conditionally-compiled code providing
   IPv6 support.  In other areas I have resolved similar warnings by
   breaking code out into functions, but in these remaining cases it'd
   be more difficult to do this without significant modifications to the
   code.  For this patchset at least I prefer to not make such invasive
   changes.

 * Two warnings about use of ENOSYS in l2tp_ppp.c.  This error code is
   returned when session-specific ioctl calls are made on a tunnel socket.
   Arguably a different code would be more appropriate; however changing
   this could be seen to be changing the userspace API.  It's unlikely
   that any code is relying on this particular error being returned in
   this scenario, but again, I prefer not to make such a change in this
   patchset.

Tom Parkin (29):
  l2tp: cleanup whitespace around operators
  l2tp: tweak comment style for consistency
  l2tp: add a blank line following declarations
  l2tp: cleanup excessive blank lines
  l2tp: cleanup difficult-to-read line breaks
  l2tp: cleanup wonky alignment of line-broken function calls
  l2tp: cleanup suspect code indent
  l2tp: add identifier name in function pointer prototype
  l2tp: prefer using BIT macro
  l2tp: cleanup comparisons to NULL
  l2tp: cleanup unnecessary braces in if statements
  l2tp: prefer seq_puts for unformatted output
  l2tp: avoid multiple assignments
  l2tp: line-break long function prototypes
  l2tp: comment per net spinlock instances
  l2tp: fix up incorrect comment in l2tp_recv_common
  l2tp: avoid precidence issues in L2TP_SKB_CB macro
  l2tp: WARN_ON rather than BUG_ON in l2tp_debugfs.c
  l2tp: use a function to render tunnel address in l2tp_debugfs
  l2tp: cleanup netlink send of tunnel address information
  l2tp: cleanup netlink tunnel create address handling
  l2tp: cleanup kzalloc calls
  l2tp: remove BUG_ON in l2tp_session_queue_purge
  l2tp: remove BUG_ON in l2tp_tunnel_closeall
  l2tp: don't BUG_ON session magic checks in l2tp_ppp
  l2tp: don't BUG_ON seqfile checks in l2tp_ppp
  l2tp: WARN_ON rather than BUG_ON in l2tp_session_queue_purge
  l2tp: remove BUG_ON refcount value in l2tp_session_free
  l2tp: WARN_ON rather than BUG_ON in l2tp_session_free

 net/l2tp/l2tp_core.c    | 123 +++++++++----------
 net/l2tp/l2tp_core.h    |  82 ++++++-------
 net/l2tp/l2tp_debugfs.c |  66 ++++++-----
 net/l2tp/l2tp_eth.c     |  19 ++-
 net/l2tp/l2tp_ip.c      |  31 +++--
 net/l2tp/l2tp_ip6.c     |  37 +++---
 net/l2tp/l2tp_netlink.c | 257 +++++++++++++++++++++-------------------
 net/l2tp/l2tp_ppp.c     |  97 ++++++++-------
 8 files changed, 362 insertions(+), 350 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 01/29] l2tp: cleanup whitespace around operators
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
@ 2020-07-21 17:31 ` Tom Parkin
  2020-07-21 17:31 ` [PATCH 02/29] l2tp: tweak comment style for consistency Tom Parkin
                   ` (28 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:31 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

checkpatch warned about whitespace around various operators in the
l2tp code.

The checkpatch suggestions help make the code more readable, so
update the l2tp code accordingly.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c | 34 +++++++++++++++++-----------------
 net/l2tp/l2tp_eth.c  | 13 ++++++-------
 net/l2tp/l2tp_ip.c   | 12 ++++++------
 net/l2tp/l2tp_ip6.c  | 14 +++++++-------
 net/l2tp/l2tp_ppp.c  |  6 +++---
 5 files changed, 39 insertions(+), 40 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 6434d17e6e8e..4031149b7d44 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -94,7 +94,7 @@ struct l2tp_skb_cb {
 	unsigned long		expires;
 };
 
-#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *) &skb->cb[sizeof(struct inet_skb_parm)])
+#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *)&skb->cb[sizeof(struct inet_skb_parm)])
 
 static struct workqueue_struct *l2tp_wq;
 
@@ -648,9 +648,9 @@ void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb,
 	L2TP_SKB_CB(skb)->has_seq = 0;
 	if (tunnel->version == L2TP_HDR_VER_2) {
 		if (hdrflags & L2TP_HDRFLAG_S) {
-			ns = ntohs(*(__be16 *) ptr);
+			ns = ntohs(*(__be16 *)ptr);
 			ptr += 2;
-			nr = ntohs(*(__be16 *) ptr);
+			nr = ntohs(*(__be16 *)ptr);
 			ptr += 2;
 
 			/* Store L2TP info in the skb */
@@ -662,7 +662,7 @@ void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb,
 				 session->name, ns, nr, session->nr);
 		}
 	} else if (session->l2specific_type == L2TP_L2SPECTYPE_DEFAULT) {
-		u32 l2h = ntohl(*(__be32 *) ptr);
+		u32 l2h = ntohl(*(__be32 *)ptr);
 
 		if (l2h & 0x40000000) {
 			ns = l2h & 0x00ffffff;
@@ -828,7 +828,7 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
 	optr = ptr = skb->data;
 
 	/* Get L2TP header flags */
-	hdrflags = ntohs(*(__be16 *) ptr);
+	hdrflags = ntohs(*(__be16 *)ptr);
 
 	/* Check protocol version */
 	version = hdrflags & L2TP_HDR_VER_MASK;
@@ -859,14 +859,14 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
 			ptr += 2;
 
 		/* Extract tunnel and session ID */
-		tunnel_id = ntohs(*(__be16 *) ptr);
+		tunnel_id = ntohs(*(__be16 *)ptr);
 		ptr += 2;
-		session_id = ntohs(*(__be16 *) ptr);
+		session_id = ntohs(*(__be16 *)ptr);
 		ptr += 2;
 	} else {
 		ptr += 2;	/* skip reserved bits */
 		tunnel_id = tunnel->tunnel_id;
-		session_id = ntohl(*(__be32 *) ptr);
+		session_id = ntohl(*(__be32 *)ptr);
 		ptr += 4;
 	}
 
@@ -971,13 +971,13 @@ static int l2tp_build_l2tpv3_header(struct l2tp_session *session, void *buf)
 	 */
 	if (tunnel->encap == L2TP_ENCAPTYPE_UDP) {
 		u16 flags = L2TP_HDR_VER_3;
-		*((__be16 *) bufp) = htons(flags);
+		*((__be16 *)bufp) = htons(flags);
 		bufp += 2;
-		*((__be16 *) bufp) = 0;
+		*((__be16 *)bufp) = 0;
 		bufp += 2;
 	}
 
-	*((__be32 *) bufp) = htonl(session->peer_session_id);
+	*((__be32 *)bufp) = htonl(session->peer_session_id);
 	bufp += 4;
 	if (session->cookie_len) {
 		memcpy(bufp, &session->cookie[0], session->cookie_len);
@@ -1305,9 +1305,9 @@ static int l2tp_tunnel_sock_create(struct net *net,
 			memcpy(&udp_conf.peer_ip6, cfg->peer_ip6,
 			       sizeof(udp_conf.peer_ip6));
 			udp_conf.use_udp6_tx_checksums =
-			  ! cfg->udp6_zero_tx_checksums;
+			  !cfg->udp6_zero_tx_checksums;
 			udp_conf.use_udp6_rx_checksums =
-			  ! cfg->udp6_zero_rx_checksums;
+			  !cfg->udp6_zero_rx_checksums;
 		} else
 #endif
 		{
@@ -1340,7 +1340,7 @@ static int l2tp_tunnel_sock_create(struct net *net,
 			memcpy(&ip6_addr.l2tp_addr, cfg->local_ip6,
 			       sizeof(ip6_addr.l2tp_addr));
 			ip6_addr.l2tp_conn_id = tunnel_id;
-			err = kernel_bind(sock, (struct sockaddr *) &ip6_addr,
+			err = kernel_bind(sock, (struct sockaddr *)&ip6_addr,
 					  sizeof(ip6_addr));
 			if (err < 0)
 				goto out;
@@ -1350,7 +1350,7 @@ static int l2tp_tunnel_sock_create(struct net *net,
 			       sizeof(ip6_addr.l2tp_addr));
 			ip6_addr.l2tp_conn_id = peer_tunnel_id;
 			err = kernel_connect(sock,
-					     (struct sockaddr *) &ip6_addr,
+					     (struct sockaddr *)&ip6_addr,
 					     sizeof(ip6_addr), 0);
 			if (err < 0)
 				goto out;
@@ -1367,7 +1367,7 @@ static int l2tp_tunnel_sock_create(struct net *net,
 			ip_addr.l2tp_family = AF_INET;
 			ip_addr.l2tp_addr = cfg->local_ip;
 			ip_addr.l2tp_conn_id = tunnel_id;
-			err = kernel_bind(sock, (struct sockaddr *) &ip_addr,
+			err = kernel_bind(sock, (struct sockaddr *)&ip_addr,
 					  sizeof(ip_addr));
 			if (err < 0)
 				goto out;
@@ -1375,7 +1375,7 @@ static int l2tp_tunnel_sock_create(struct net *net,
 			ip_addr.l2tp_family = AF_INET;
 			ip_addr.l2tp_addr = cfg->peer_ip;
 			ip_addr.l2tp_conn_id = peer_tunnel_id;
-			err = kernel_connect(sock, (struct sockaddr *) &ip_addr,
+			err = kernel_connect(sock, (struct sockaddr *)&ip_addr,
 					     sizeof(ip_addr), 0);
 			if (err < 0)
 				goto out;
diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index 3099efa19249..86e111d7d73c 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -94,13 +94,12 @@ static void l2tp_eth_get_stats64(struct net_device *dev,
 {
 	struct l2tp_eth *priv = netdev_priv(dev);
 
-	stats->tx_bytes   = (unsigned long) atomic_long_read(&priv->tx_bytes);
-	stats->tx_packets = (unsigned long) atomic_long_read(&priv->tx_packets);
-	stats->tx_dropped = (unsigned long) atomic_long_read(&priv->tx_dropped);
-	stats->rx_bytes   = (unsigned long) atomic_long_read(&priv->rx_bytes);
-	stats->rx_packets = (unsigned long) atomic_long_read(&priv->rx_packets);
-	stats->rx_errors  = (unsigned long) atomic_long_read(&priv->rx_errors);
-
+	stats->tx_bytes   = (unsigned long)atomic_long_read(&priv->tx_bytes);
+	stats->tx_packets = (unsigned long)atomic_long_read(&priv->tx_packets);
+	stats->tx_dropped = (unsigned long)atomic_long_read(&priv->tx_dropped);
+	stats->rx_bytes   = (unsigned long)atomic_long_read(&priv->rx_bytes);
+	stats->rx_packets = (unsigned long)atomic_long_read(&priv->rx_packets);
+	stats->rx_errors  = (unsigned long)atomic_long_read(&priv->rx_errors);
 }
 
 static const struct net_device_ops l2tp_eth_netdev_ops = {
diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
index 2a3fd31fb589..c52eb2510ca4 100644
--- a/net/l2tp/l2tp_ip.c
+++ b/net/l2tp/l2tp_ip.c
@@ -126,7 +126,7 @@ static int l2tp_ip_recv(struct sk_buff *skb)
 
 	/* Point to L2TP header */
 	optr = ptr = skb->data;
-	session_id = ntohl(*((__be32 *) ptr));
+	session_id = ntohl(*((__be32 *)ptr));
 	ptr += 4;
 
 	/* RFC3931: L2TP/IP packets have the first 4 bytes containing
@@ -176,7 +176,7 @@ static int l2tp_ip_recv(struct sk_buff *skb)
 	if ((skb->data[0] & 0xc0) != 0xc0)
 		goto discard;
 
-	tunnel_id = ntohl(*(__be32 *) &skb->data[4]);
+	tunnel_id = ntohl(*(__be32 *)&skb->data[4]);
 	iph = (struct iphdr *)skb_network_header(skb);
 
 	read_lock_bh(&l2tp_ip_lock);
@@ -260,7 +260,7 @@ static void l2tp_ip_destroy_sock(struct sock *sk)
 static int l2tp_ip_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 {
 	struct inet_sock *inet = inet_sk(sk);
-	struct sockaddr_l2tpip *addr = (struct sockaddr_l2tpip *) uaddr;
+	struct sockaddr_l2tpip *addr = (struct sockaddr_l2tpip *)uaddr;
 	struct net *net = sock_net(sk);
 	int ret;
 	int chk_addr_ret;
@@ -316,7 +316,7 @@ static int l2tp_ip_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 
 static int l2tp_ip_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 {
-	struct sockaddr_l2tpip *lsa = (struct sockaddr_l2tpip *) uaddr;
+	struct sockaddr_l2tpip *lsa = (struct sockaddr_l2tpip *)uaddr;
 	int rc;
 
 	if (addr_len < sizeof(*lsa))
@@ -456,7 +456,7 @@ static int l2tp_ip_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	skb_reset_transport_header(skb);
 
 	/* Insert 0 session_id */
-	*((__be32 *) skb_put(skb, 4)) = 0;
+	*((__be32 *)skb_put(skb, 4)) = 0;
 
 	/* Copy user data into skb */
 	rc = memcpy_from_msg(skb_put(skb, len), msg, len);
@@ -467,7 +467,7 @@ static int l2tp_ip_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 
 	fl4 = &inet->cork.fl.u.ip4;
 	if (connected)
-		rt = (struct rtable *) __sk_dst_check(sk, 0);
+		rt = (struct rtable *)__sk_dst_check(sk, 0);
 
 	rcu_read_lock();
 	if (rt == NULL) {
diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index 4799bec87b33..d17b9fe1180f 100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c
@@ -138,7 +138,7 @@ static int l2tp_ip6_recv(struct sk_buff *skb)
 
 	/* Point to L2TP header */
 	optr = ptr = skb->data;
-	session_id = ntohl(*((__be32 *) ptr));
+	session_id = ntohl(*((__be32 *)ptr));
 	ptr += 4;
 
 	/* RFC3931: L2TP/IP packets have the first 4 bytes containing
@@ -188,7 +188,7 @@ static int l2tp_ip6_recv(struct sk_buff *skb)
 	if ((skb->data[0] & 0xc0) != 0xc0)
 		goto discard;
 
-	tunnel_id = ntohl(*(__be32 *) &skb->data[4]);
+	tunnel_id = ntohl(*(__be32 *)&skb->data[4]);
 	iph = ipv6_hdr(skb);
 
 	read_lock_bh(&l2tp_ip6_lock);
@@ -276,7 +276,7 @@ static int l2tp_ip6_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 {
 	struct inet_sock *inet = inet_sk(sk);
 	struct ipv6_pinfo *np = inet6_sk(sk);
-	struct sockaddr_l2tpip6 *addr = (struct sockaddr_l2tpip6 *) uaddr;
+	struct sockaddr_l2tpip6 *addr = (struct sockaddr_l2tpip6 *)uaddr;
 	struct net *net = sock_net(sk);
 	__be32 v4addr = 0;
 	int bound_dev_if;
@@ -375,8 +375,8 @@ static int l2tp_ip6_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 static int l2tp_ip6_connect(struct sock *sk, struct sockaddr *uaddr,
 			    int addr_len)
 {
-	struct sockaddr_l2tpip6 *lsa = (struct sockaddr_l2tpip6 *) uaddr;
-	struct sockaddr_in6	*usin = (struct sockaddr_in6 *) uaddr;
+	struct sockaddr_l2tpip6 *lsa = (struct sockaddr_l2tpip6 *)uaddr;
+	struct sockaddr_in6	*usin = (struct sockaddr_in6 *)uaddr;
 	struct in6_addr	*daddr;
 	int	addr_type;
 	int rc;
@@ -548,7 +548,7 @@ static int l2tp_ip6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 		daddr = &lsa->l2tp_addr;
 		if (np->sndflow) {
 			fl6.flowlabel = lsa->l2tp_flowinfo & IPV6_FLOWINFO_MASK;
-			if (fl6.flowlabel&IPV6_FLOWLABEL_MASK) {
+			if (fl6.flowlabel & IPV6_FLOWLABEL_MASK) {
 				flowlabel = fl6_sock_lookup(sk, fl6.flowlabel);
 				if (IS_ERR(flowlabel))
 					return -EINVAL;
@@ -594,7 +594,7 @@ static int l2tp_ip6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 			if (IS_ERR(flowlabel))
 				return -EINVAL;
 		}
-		if (!(opt->opt_nflen|opt->opt_flen))
+		if (!(opt->opt_nflen | opt->opt_flen))
 			opt = NULL;
 	}
 
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index c54cb59593ef..a0bd39f0d5fe 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -351,7 +351,7 @@ static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m,
  */
 static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
 {
-	struct sock *sk = (struct sock *) chan->private;
+	struct sock *sk = (struct sock *)chan->private;
 	struct l2tp_session *session;
 	struct l2tp_tunnel *tunnel;
 	int uhlen, headroom;
@@ -1343,7 +1343,7 @@ static int pppol2tp_session_getsockopt(struct sock *sk,
 		break;
 
 	case PPPOL2TP_SO_REORDERTO:
-		*val = (int) jiffies_to_msecs(session->reorder_timeout);
+		*val = (int)jiffies_to_msecs(session->reorder_timeout);
 		l2tp_info(session, L2TP_MSG_CONTROL,
 			  "%s: get reorder_timeout=%d\n", session->name, *val);
 		break;
@@ -1407,7 +1407,7 @@ static int pppol2tp_getsockopt(struct socket *sock, int level, int optname,
 	if (put_user(len, optlen))
 		goto end_put_sess;
 
-	if (copy_to_user((void __user *) optval, &val, len))
+	if (copy_to_user((void __user *)optval, &val, len))
 		goto end_put_sess;
 
 	err = 0;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 02/29] l2tp: tweak comment style for consistency
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
  2020-07-21 17:31 ` [PATCH 01/29] l2tp: cleanup whitespace around operators Tom Parkin
@ 2020-07-21 17:31 ` Tom Parkin
  2020-07-21 17:31 ` [PATCH 03/29] l2tp: add a blank line following declarations Tom Parkin
                   ` (27 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:31 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

Modify some l2tp comments to better adhere to kernel coding style.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c    |  5 ++-
 net/l2tp/l2tp_core.h    | 76 +++++++++++++++++------------------------
 net/l2tp/l2tp_debugfs.c |  3 +-
 net/l2tp/l2tp_eth.c     |  3 +-
 net/l2tp/l2tp_ip.c      |  3 +-
 net/l2tp/l2tp_ip6.c     | 15 ++++----
 net/l2tp/l2tp_netlink.c |  3 +-
 net/l2tp/l2tp_ppp.c     |  3 +-
 8 files changed, 44 insertions(+), 67 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 4031149b7d44..14f4ae6c5b0f 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1,6 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0-only
-/*
- * L2TP core.
+/* L2TP core.
  *
  * Copyright (c) 2008,2009,2010 Katalix Systems Ltd
  *
@@ -1603,7 +1602,7 @@ void __l2tp_session_unhash(struct l2tp_session *session)
 EXPORT_SYMBOL_GPL(__l2tp_session_unhash);
 
 /* This function is used by the netlink SESSION_DELETE command and by
-   pseudowire modules.
+ * pseudowire modules.
  */
 int l2tp_session_delete(struct l2tp_session *session)
 {
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 10cf7c3dcbb3..3ebb701eebbf 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -1,6 +1,5 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * L2TP internal definitions.
+/* L2TP internal definitions.
  *
  * Copyright (c) 2008,2009 Katalix Systems Ltd
  */
@@ -49,32 +48,26 @@ struct l2tp_tunnel;
  */
 struct l2tp_session_cfg {
 	enum l2tp_pwtype	pw_type;
-	unsigned int		recv_seq:1;	/* expect receive packets with
-						 * sequence numbers? */
-	unsigned int		send_seq:1;	/* send packets with sequence
-						 * numbers? */
-	unsigned int		lns_mode:1;	/* behave as LNS? LAC enables
-						 * sequence numbers under
-						 * control of LNS. */
-	int			debug;		/* bitmask of debug message
-						 * categories */
+	unsigned int		recv_seq:1;	/* expect receive packets with sequence numbers? */
+	unsigned int		send_seq:1;	/* send packets with sequence numbers? */
+	unsigned int		lns_mode:1;	/* behave as LNS?
+						 * LAC enables sequence numbers under LNS control.
+						 */
+	int			debug;		/* bitmask of debug message categories */
 	u16			l2specific_type; /* Layer 2 specific type */
 	u8			cookie[8];	/* optional cookie */
 	int			cookie_len;	/* 0, 4 or 8 bytes */
 	u8			peer_cookie[8];	/* peer's cookie */
 	int			peer_cookie_len; /* 0, 4 or 8 bytes */
-	int			reorder_timeout; /* configured reorder timeout
-						  * (in jiffies) */
+	int			reorder_timeout; /* configured reorder timeout (in jiffies) */
 	char			*ifname;
 };
 
 struct l2tp_session {
-	int			magic;		/* should be
-						 * L2TP_SESSION_MAGIC */
+	int			magic;		/* should be L2TP_SESSION_MAGIC */
 	long			dead;
 
-	struct l2tp_tunnel	*tunnel;	/* back pointer to tunnel
-						 * context */
+	struct l2tp_tunnel	*tunnel;	/* back pointer to tunnel context */
 	u32			session_id;
 	u32			peer_session_id;
 	u8			cookie[8];
@@ -89,42 +82,37 @@ struct l2tp_session {
 	u32			nr_max;		/* max NR. Depends on tunnel */
 	u32			nr_window_size;	/* NR window size */
 	u32			nr_oos;		/* NR of last OOS packet */
-	int			nr_oos_count;	/* For OOS recovery */
+	int			nr_oos_count;	/* for OOS recovery */
 	int			nr_oos_count_max;
-	struct hlist_node	hlist;		/* Hash list node */
+	struct hlist_node	hlist;		/* hash list node */
 	refcount_t		ref_count;
 
 	char			name[32];	/* for logging */
 	char			ifname[IFNAMSIZ];
-	unsigned int		recv_seq:1;	/* expect receive packets with
-						 * sequence numbers? */
-	unsigned int		send_seq:1;	/* send packets with sequence
-						 * numbers? */
-	unsigned int		lns_mode:1;	/* behave as LNS? LAC enables
-						 * sequence numbers under
-						 * control of LNS. */
-	int			debug;		/* bitmask of debug message
-						 * categories */
-	int			reorder_timeout; /* configured reorder timeout
-						  * (in jiffies) */
+	unsigned int		recv_seq:1;	/* expect receive packets with sequence numbers? */
+	unsigned int		send_seq:1;	/* send packets with sequence numbers? */
+	unsigned int		lns_mode:1;	/* behave as LNS?
+						 * LAC enables sequence numbers under LNS control.
+						 */
+	int			debug;		/* bitmask of debug message categories */
+	int			reorder_timeout; /* configured reorder timeout (in jiffies) */
 	int			reorder_skip;	/* set if skip to next nr */
 	enum l2tp_pwtype	pwtype;
 	struct l2tp_stats	stats;
-	struct hlist_node	global_hlist;	/* Global hash list node */
+	struct hlist_node	global_hlist;	/* global hash list node */
 
 	int (*build_header)(struct l2tp_session *session, void *buf);
 	void (*recv_skb)(struct l2tp_session *session, struct sk_buff *skb, int data_len);
 	void (*session_close)(struct l2tp_session *session);
 	void (*show)(struct seq_file *m, void *priv);
-	u8			priv[];	/* private data */
+	u8			priv[];		/* private data */
 };
 
 /* Describes the tunnel. It contains info to track all the associated
  * sessions so incoming packets can be sorted out
  */
 struct l2tp_tunnel_cfg {
-	int			debug;		/* bitmask of debug message
-						 * categories */
+	int			debug;		/* bitmask of debug message categories */
 	enum l2tp_encap_type	encap;
 
 	/* Used only for kernel-created sockets */
@@ -148,31 +136,29 @@ struct l2tp_tunnel {
 
 	struct rcu_head rcu;
 	rwlock_t		hlist_lock;	/* protect session_hlist */
-	bool			acpt_newsess;	/* Indicates whether this
-						 * tunnel accepts new sessions.
-						 * Protected by hlist_lock.
+	bool			acpt_newsess;	/* indicates whether this tunnel accepts
+						 * new sessions. Protected by hlist_lock.
 						 */
 	struct hlist_head	session_hlist[L2TP_HASH_SIZE];
-						/* hashed list of sessions,
-						 * hashed by id */
+						/* hashed list of sessions, hashed by id */
 	u32			tunnel_id;
 	u32			peer_tunnel_id;
 	int			version;	/* 2=>L2TPv2, 3=>L2TPv3 */
 
 	char			name[20];	/* for logging */
-	int			debug;		/* bitmask of debug message
-						 * categories */
+	int			debug;		/* bitmask of debug message categories */
 	enum l2tp_encap_type	encap;
 	struct l2tp_stats	stats;
 
-	struct list_head	list;		/* Keep a list of all tunnels */
+	struct list_head	list;		/* list node on per-namespace list of tunnels */
 	struct net		*l2tp_net;	/* the net we belong to */
 
 	refcount_t		ref_count;
 	void (*old_sk_destruct)(struct sock *);
-	struct sock		*sock;		/* Parent socket */
-	int			fd;		/* Parent fd, if tunnel socket
-						 * was created by userspace */
+	struct sock		*sock;		/* parent socket */
+	int			fd;		/* parent fd, if tunnel socket was created
+						 * by userspace
+						 */
 
 	struct work_struct	del_work;
 };
diff --git a/net/l2tp/l2tp_debugfs.c b/net/l2tp/l2tp_debugfs.c
index 35bb4f3bdbe0..221b86e1ba7c 100644
--- a/net/l2tp/l2tp_debugfs.c
+++ b/net/l2tp/l2tp_debugfs.c
@@ -1,6 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * L2TP subsystem debugfs
+/* L2TP subsystem debugfs
  *
  * Copyright (c) 2010 Katalix Systems Ltd
  */
diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index 86e111d7d73c..87e4cebc7f57 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -1,6 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * L2TPv3 ethernet pseudowire driver
+/* L2TPv3 ethernet pseudowire driver
  *
  * Copyright (c) 2008,2009,2010 Katalix Systems Ltd
  */
diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
index c52eb2510ca4..65cf5a1a1e08 100644
--- a/net/l2tp/l2tp_ip.c
+++ b/net/l2tp/l2tp_ip.c
@@ -1,6 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * L2TPv3 IP encapsulation support
+/* L2TPv3 IP encapsulation support
  *
  * Copyright (c) 2008,2009,2010 Katalix Systems Ltd
  */
diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index d17b9fe1180f..ca7696147c7e 100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c
@@ -1,6 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * L2TPv3 IP encapsulation support for IPv6
+/* L2TPv3 IP encapsulation support for IPv6
  *
  * Copyright (c) 2012 Katalix Systems Ltd
  */
@@ -38,7 +37,8 @@ struct l2tp_ip6_sock {
 	u32			peer_conn_id;
 
 	/* ipv6_pinfo has to be the last member of l2tp_ip6_sock, see
-	   inet6_sk_generic */
+	 * inet6_sk_generic
+	 */
 	struct ipv6_pinfo	inet6;
 };
 
@@ -519,7 +519,7 @@ static int l2tp_ip6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	int err;
 
 	/* Rough check on arithmetic overflow,
-	   better check is made in ip6_append_data().
+	 * better check is made in ip6_append_data().
 	 */
 	if (len > INT_MAX)
 		return -EMSGSIZE;
@@ -528,9 +528,7 @@ static int l2tp_ip6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	if (msg->msg_flags & MSG_OOB)
 		return -EOPNOTSUPP;
 
-	/*
-	 *	Get and verify the address.
-	 */
+	/* Get and verify the address */
 	memset(&fl6, 0, sizeof(fl6));
 
 	fl6.flowi6_mark = sk->sk_mark;
@@ -555,8 +553,7 @@ static int l2tp_ip6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 			}
 		}
 
-		/*
-		 * Otherwise it will be difficult to maintain
+		/* Otherwise it will be difficult to maintain
 		 * sk->sk_dst_cache.
 		 */
 		if (sk->sk_state == TCP_ESTABLISHED &&
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index ebb381c3f1b9..7643378ebead 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -1,6 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0-only
-/*
- * L2TP netlink layer, for management
+/* L2TP netlink layer, for management
  *
  * Copyright (c) 2008,2009,2010 Katalix Systems Ltd
  *
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index a0bd39f0d5fe..e0dd56fef018 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -117,8 +117,7 @@ struct pppol2tp_session {
 	int			owner;		/* pid that opened the socket */
 
 	struct mutex		sk_lock;	/* Protects .sk */
-	struct sock __rcu	*sk;		/* Pointer to the session
-						 * PPPoX socket */
+	struct sock __rcu	*sk;		/* Pointer to the session PPPoX socket */
 	struct sock		*__sk;		/* Copy of .sk, for cleanup */
 	struct rcu_head		rcu;		/* For asynchronous release */
 };
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 03/29] l2tp: add a blank line following declarations
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
  2020-07-21 17:31 ` [PATCH 01/29] l2tp: cleanup whitespace around operators Tom Parkin
  2020-07-21 17:31 ` [PATCH 02/29] l2tp: tweak comment style for consistency Tom Parkin
@ 2020-07-21 17:31 ` Tom Parkin
  2020-07-21 17:31 ` [PATCH 04/29] l2tp: cleanup excessive blank lines Tom Parkin
                   ` (26 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:31 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

checkpatch likes an empty line following declarations, which gives a
visual prompt for where logic begins in a given block.

Add blank lines in l2tp code to adhere to this guideline.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c    | 2 ++
 net/l2tp/l2tp_ip.c      | 2 ++
 net/l2tp/l2tp_netlink.c | 2 ++
 net/l2tp/l2tp_ppp.c     | 3 +++
 4 files changed, 9 insertions(+)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 14f4ae6c5b0f..3308e84906ef 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -776,6 +776,7 @@ EXPORT_SYMBOL(l2tp_recv_common);
 static int l2tp_session_queue_purge(struct l2tp_session *session)
 {
 	struct sk_buff *skb = NULL;
+
 	BUG_ON(!session);
 	BUG_ON(session->magic != L2TP_SESSION_MAGIC);
 	while ((skb = skb_dequeue(&session->reorder_q))) {
@@ -1592,6 +1593,7 @@ void __l2tp_session_unhash(struct l2tp_session *session)
 		/* For L2TPv3 we have a per-net hash: remove from there, too */
 		if (tunnel->version != L2TP_HDR_VER_2) {
 			struct l2tp_net *pn = l2tp_pernet(tunnel->l2tp_net);
+
 			spin_lock_bh(&pn->l2tp_session_hlist_lock);
 			hlist_del_init_rcu(&session->global_hlist);
 			spin_unlock_bh(&pn->l2tp_session_hlist_lock);
diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
index 65cf5a1a1e08..70f9fdaf6c86 100644
--- a/net/l2tp/l2tp_ip.c
+++ b/net/l2tp/l2tp_ip.c
@@ -374,6 +374,7 @@ static int l2tp_ip_getname(struct socket *sock, struct sockaddr *uaddr,
 		lsa->l2tp_addr.s_addr = inet->inet_daddr;
 	} else {
 		__be32 addr = inet->inet_rcv_saddr;
+
 		if (!addr)
 			addr = inet->inet_saddr;
 		lsa->l2tp_conn_id = lsk->conn_id;
@@ -421,6 +422,7 @@ static int l2tp_ip_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	/* Get and verify the address. */
 	if (msg->msg_name) {
 		DECLARE_SOCKADDR(struct sockaddr_l2tpip *, lip, msg->msg_name);
+
 		rc = -EINVAL;
 		if (msg->msg_namelen < sizeof(*lip))
 			goto out;
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 7643378ebead..5b24efc0b04b 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -569,6 +569,7 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
 
 		if (info->attrs[L2TP_ATTR_COOKIE]) {
 			u16 len = nla_len(info->attrs[L2TP_ATTR_COOKIE]);
+
 			if (len > 8) {
 				ret = -EINVAL;
 				goto out_tunnel;
@@ -578,6 +579,7 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
 		}
 		if (info->attrs[L2TP_ATTR_PEER_COOKIE]) {
 			u16 len = nla_len(info->attrs[L2TP_ATTR_PEER_COOKIE]);
+
 			if (len > 8) {
 				ret = -EINVAL;
 				goto out_tunnel;
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index e0dd56fef018..48fbaf5ee82c 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -927,6 +927,7 @@ static int pppol2tp_getname(struct socket *sock, struct sockaddr *uaddr,
 	inet = inet_sk(tunnel->sock);
 	if ((tunnel->version == 2) && (tunnel->sock->sk_family == AF_INET)) {
 		struct sockaddr_pppol2tp sp;
+
 		len = sizeof(sp);
 		memset(&sp, 0, len);
 		sp.sa_family	= AF_PPPOX;
@@ -983,6 +984,7 @@ static int pppol2tp_getname(struct socket *sock, struct sockaddr *uaddr,
 #endif
 	} else if (tunnel->version == 3) {
 		struct sockaddr_pppol2tpv3 sp;
+
 		len = sizeof(sp);
 		memset(&sp, 0, len);
 		sp.sa_family	= AF_PPPOX;
@@ -1550,6 +1552,7 @@ static void pppol2tp_seq_session_show(struct seq_file *m, void *v)
 
 	if (tunnel->sock) {
 		struct inet_sock *inet = inet_sk(tunnel->sock);
+
 		ip = ntohl(inet->inet_saddr);
 		port = ntohs(inet->inet_sport);
 	}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 04/29] l2tp: cleanup excessive blank lines
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (2 preceding siblings ...)
  2020-07-21 17:31 ` [PATCH 03/29] l2tp: add a blank line following declarations Tom Parkin
@ 2020-07-21 17:31 ` Tom Parkin
  2020-07-21 17:31 ` [PATCH 05/29] l2tp: cleanup difficult-to-read line breaks Tom Parkin
                   ` (25 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:31 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

checkpatch doesn't like multiple blank lines, or trailing blank lines at
the end of functions.  They serve no useful purpose, so remove these from
the l2tp code.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c    | 2 --
 net/l2tp/l2tp_debugfs.c | 2 --
 net/l2tp/l2tp_eth.c     | 3 ---
 net/l2tp/l2tp_netlink.c | 1 -
 4 files changed, 8 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 3308e84906ef..70891be75f77 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -133,7 +133,6 @@ static inline struct hlist_head *
 l2tp_session_id_hash_2(struct l2tp_net *pn, u32 session_id)
 {
 	return &pn->l2tp_session_hlist[hash_32(session_id, L2TP_HASH_BITS_2)];
-
 }
 
 /* Session hash list.
@@ -1637,7 +1636,6 @@ void l2tp_session_set_header_len(struct l2tp_session *session, int version)
 		if (session->tunnel->encap == L2TP_ENCAPTYPE_UDP)
 			session->hdr_len += 4;
 	}
-
 }
 EXPORT_SYMBOL_GPL(l2tp_session_set_header_len);
 
diff --git a/net/l2tp/l2tp_debugfs.c b/net/l2tp/l2tp_debugfs.c
index 221b86e1ba7c..93181133e155 100644
--- a/net/l2tp/l2tp_debugfs.c
+++ b/net/l2tp/l2tp_debugfs.c
@@ -62,7 +62,6 @@ static void l2tp_dfs_next_session(struct l2tp_dfs_seq_data *pd)
 		pd->session_idx = 0;
 		l2tp_dfs_next_tunnel(pd);
 	}
-
 }
 
 static void *l2tp_dfs_seq_start(struct seq_file *m, loff_t *offs)
@@ -89,7 +88,6 @@ static void *l2tp_dfs_seq_start(struct seq_file *m, loff_t *offs)
 	return pd;
 }
 
-
 static void *l2tp_dfs_seq_next(struct seq_file *m, void *v, loff_t *pos)
 {
 	(*pos)++;
diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index 87e4cebc7f57..7ed2b4eced94 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -50,7 +50,6 @@ struct l2tp_eth_sess {
 	struct net_device __rcu *dev;
 };
 
-
 static int l2tp_eth_dev_init(struct net_device *dev)
 {
 	eth_hw_addr_random(dev);
@@ -346,13 +345,11 @@ static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
 	return rc;
 }
 
-
 static const struct l2tp_nl_cmd_ops l2tp_eth_nl_cmd_ops = {
 	.session_create	= l2tp_eth_create,
 	.session_delete	= l2tp_session_delete,
 };
 
-
 static int __init l2tp_eth_init(void)
 {
 	int err = 0;
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 5b24efc0b04b..3120f8dcc56a 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -26,7 +26,6 @@
 
 #include "l2tp_core.h"
 
-
 static struct genl_family l2tp_nl_family;
 
 static const struct genl_multicast_group l2tp_multicast_group[] = {
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 05/29] l2tp: cleanup difficult-to-read line breaks
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (3 preceding siblings ...)
  2020-07-21 17:31 ` [PATCH 04/29] l2tp: cleanup excessive blank lines Tom Parkin
@ 2020-07-21 17:31 ` Tom Parkin
  2020-07-21 20:59   ` Jakub Kicinski
  2020-07-21 17:31 ` [PATCH 06/29] l2tp: cleanup wonky alignment of line-broken function calls Tom Parkin
                   ` (24 subsequent siblings)
  29 siblings, 1 reply; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:31 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

Some l2tp code had line breaks which made the code more difficult to
read.  These were originally motivated by the 80-character line width
coding guidelines, but were actually a negative from the perspective of
trying to follow the code.

Remove these linebreaks for clearer code, even if we do exceed 80
characters in width in some places.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_netlink.c | 71 +++++++++++++++++------------------------
 net/l2tp/l2tp_ppp.c     |  6 ++--
 2 files changed, 32 insertions(+), 45 deletions(-)

diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 3120f8dcc56a..0325ed2cfe8a 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -165,71 +165,63 @@ static int l2tp_nl_cmd_tunnel_create(struct sk_buff *skb, struct genl_info *info
 	struct l2tp_tunnel_cfg cfg = { 0, };
 	struct l2tp_tunnel *tunnel;
 	struct net *net = genl_info_net(info);
+	struct nlattr **attrs = info->attrs;
 
-	if (!info->attrs[L2TP_ATTR_CONN_ID]) {
+	if (!attrs[L2TP_ATTR_CONN_ID]) {
 		ret = -EINVAL;
 		goto out;
 	}
-	tunnel_id = nla_get_u32(info->attrs[L2TP_ATTR_CONN_ID]);
+	tunnel_id = nla_get_u32(attrs[L2TP_ATTR_CONN_ID]);
 
-	if (!info->attrs[L2TP_ATTR_PEER_CONN_ID]) {
+	if (!attrs[L2TP_ATTR_PEER_CONN_ID]) {
 		ret = -EINVAL;
 		goto out;
 	}
-	peer_tunnel_id = nla_get_u32(info->attrs[L2TP_ATTR_PEER_CONN_ID]);
+	peer_tunnel_id = nla_get_u32(attrs[L2TP_ATTR_PEER_CONN_ID]);
 
-	if (!info->attrs[L2TP_ATTR_PROTO_VERSION]) {
+	if (!attrs[L2TP_ATTR_PROTO_VERSION]) {
 		ret = -EINVAL;
 		goto out;
 	}
-	proto_version = nla_get_u8(info->attrs[L2TP_ATTR_PROTO_VERSION]);
+	proto_version = nla_get_u8(attrs[L2TP_ATTR_PROTO_VERSION]);
 
-	if (!info->attrs[L2TP_ATTR_ENCAP_TYPE]) {
+	if (!attrs[L2TP_ATTR_ENCAP_TYPE]) {
 		ret = -EINVAL;
 		goto out;
 	}
-	cfg.encap = nla_get_u16(info->attrs[L2TP_ATTR_ENCAP_TYPE]);
+	cfg.encap = nla_get_u16(attrs[L2TP_ATTR_ENCAP_TYPE]);
 
 	fd = -1;
-	if (info->attrs[L2TP_ATTR_FD]) {
-		fd = nla_get_u32(info->attrs[L2TP_ATTR_FD]);
+	if (attrs[L2TP_ATTR_FD]) {
+		fd = nla_get_u32(attrs[L2TP_ATTR_FD]);
 	} else {
 #if IS_ENABLED(CONFIG_IPV6)
-		if (info->attrs[L2TP_ATTR_IP6_SADDR] &&
-		    info->attrs[L2TP_ATTR_IP6_DADDR]) {
-			cfg.local_ip6 = nla_data(
-				info->attrs[L2TP_ATTR_IP6_SADDR]);
-			cfg.peer_ip6 = nla_data(
-				info->attrs[L2TP_ATTR_IP6_DADDR]);
-		} else
+		if (attrs[L2TP_ATTR_IP6_SADDR] && attrs[L2TP_ATTR_IP6_DADDR]) {
+			cfg.local_ip6 = nla_data(attrs[L2TP_ATTR_IP6_SADDR]);
+			cfg.peer_ip6 = nla_data(attrs[L2TP_ATTR_IP6_DADDR]);
+		} else {
 #endif
-		if (info->attrs[L2TP_ATTR_IP_SADDR] &&
-		    info->attrs[L2TP_ATTR_IP_DADDR]) {
-			cfg.local_ip.s_addr = nla_get_in_addr(
-				info->attrs[L2TP_ATTR_IP_SADDR]);
-			cfg.peer_ip.s_addr = nla_get_in_addr(
-				info->attrs[L2TP_ATTR_IP_DADDR]);
+		if (attrs[L2TP_ATTR_IP_SADDR] && attrs[L2TP_ATTR_IP_DADDR]) {
+			cfg.local_ip.s_addr = nla_get_in_addr(attrs[L2TP_ATTR_IP_SADDR]);
+			cfg.peer_ip.s_addr = nla_get_in_addr(attrs[L2TP_ATTR_IP_DADDR]);
 		} else {
 			ret = -EINVAL;
 			goto out;
 		}
-		if (info->attrs[L2TP_ATTR_UDP_SPORT])
-			cfg.local_udp_port = nla_get_u16(info->attrs[L2TP_ATTR_UDP_SPORT]);
-		if (info->attrs[L2TP_ATTR_UDP_DPORT])
-			cfg.peer_udp_port = nla_get_u16(info->attrs[L2TP_ATTR_UDP_DPORT]);
-		cfg.use_udp_checksums = nla_get_flag(
-			info->attrs[L2TP_ATTR_UDP_CSUM]);
+		if (attrs[L2TP_ATTR_UDP_SPORT])
+			cfg.local_udp_port = nla_get_u16(attrs[L2TP_ATTR_UDP_SPORT]);
+		if (attrs[L2TP_ATTR_UDP_DPORT])
+			cfg.peer_udp_port = nla_get_u16(attrs[L2TP_ATTR_UDP_DPORT]);
+		cfg.use_udp_checksums = nla_get_flag(attrs[L2TP_ATTR_UDP_CSUM]);
 
 #if IS_ENABLED(CONFIG_IPV6)
-		cfg.udp6_zero_tx_checksums = nla_get_flag(
-			info->attrs[L2TP_ATTR_UDP_ZERO_CSUM6_TX]);
-		cfg.udp6_zero_rx_checksums = nla_get_flag(
-			info->attrs[L2TP_ATTR_UDP_ZERO_CSUM6_RX]);
+		cfg.udp6_zero_tx_checksums = nla_get_flag(attrs[L2TP_ATTR_UDP_ZERO_CSUM6_TX]);
+		cfg.udp6_zero_rx_checksums = nla_get_flag(attrs[L2TP_ATTR_UDP_ZERO_CSUM6_RX]);
 #endif
 	}
 
-	if (info->attrs[L2TP_ATTR_DEBUG])
-		cfg.debug = nla_get_u32(info->attrs[L2TP_ATTR_DEBUG]);
+	if (attrs[L2TP_ATTR_DEBUG])
+		cfg.debug = nla_get_u32(attrs[L2TP_ATTR_DEBUG]);
 
 	ret = -EINVAL;
 	switch (cfg.encap) {
@@ -715,8 +707,7 @@ static int l2tp_nl_session_send(struct sk_buff *skb, u32 portid, u32 seq, int fl
 	if (nla_put_u32(skb, L2TP_ATTR_CONN_ID, tunnel->tunnel_id) ||
 	    nla_put_u32(skb, L2TP_ATTR_SESSION_ID, session->session_id) ||
 	    nla_put_u32(skb, L2TP_ATTR_PEER_CONN_ID, tunnel->peer_tunnel_id) ||
-	    nla_put_u32(skb, L2TP_ATTR_PEER_SESSION_ID,
-			session->peer_session_id) ||
+	    nla_put_u32(skb, L2TP_ATTR_PEER_SESSION_ID, session->peer_session_id) ||
 	    nla_put_u32(skb, L2TP_ATTR_DEBUG, session->debug) ||
 	    nla_put_u16(skb, L2TP_ATTR_PW_TYPE, session->pwtype))
 		goto nla_put_failure;
@@ -724,11 +715,9 @@ static int l2tp_nl_session_send(struct sk_buff *skb, u32 portid, u32 seq, int fl
 	if ((session->ifname[0] &&
 	     nla_put_string(skb, L2TP_ATTR_IFNAME, session->ifname)) ||
 	    (session->cookie_len &&
-	     nla_put(skb, L2TP_ATTR_COOKIE, session->cookie_len,
-		     &session->cookie[0])) ||
+	     nla_put(skb, L2TP_ATTR_COOKIE, session->cookie_len, session->cookie)) ||
 	    (session->peer_cookie_len &&
-	     nla_put(skb, L2TP_ATTR_PEER_COOKIE, session->peer_cookie_len,
-		     &session->peer_cookie[0])) ||
+	     nla_put(skb, L2TP_ATTR_PEER_COOKIE, session->peer_cookie_len, session->peer_cookie)) ||
 	    nla_put_u8(skb, L2TP_ATTR_RECV_SEQ, session->recv_seq) ||
 	    nla_put_u8(skb, L2TP_ATTR_SEND_SEQ, session->send_seq) ||
 	    nla_put_u8(skb, L2TP_ATTR_LNS_MODE, session->lns_mode) ||
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 48fbaf5ee82c..3fed922addb5 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -1566,8 +1566,7 @@ static void pppol2tp_seq_session_show(struct seq_file *m, void *v)
 		user_data_ok = 'N';
 	}
 
-	seq_printf(m, "  SESSION '%s' %08X/%d %04X/%04X -> "
-		   "%04X/%04X %d %c\n",
+	seq_printf(m, "  SESSION '%s' %08X/%d %04X/%04X -> %04X/%04X %d %c\n",
 		   session->name, ip, port,
 		   tunnel->tunnel_id,
 		   session->session_id,
@@ -1606,8 +1605,7 @@ static int pppol2tp_seq_show(struct seq_file *m, void *v)
 		seq_puts(m, "PPPoL2TP driver info, " PPPOL2TP_DRV_VERSION "\n");
 		seq_puts(m, "TUNNEL name, user-data-ok session-count\n");
 		seq_puts(m, " debug tx-pkts/bytes/errs rx-pkts/bytes/errs\n");
-		seq_puts(m, "  SESSION name, addr/port src-tid/sid "
-			 "dest-tid/sid state user-data-ok\n");
+		seq_puts(m, "  SESSION name, addr/port src-tid/sid dest-tid/sid state user-data-ok\n");
 		seq_puts(m, "   mtu/mru/rcvseq/sendseq/lns debug reorderto\n");
 		seq_puts(m, "   nr/ns tx-pkts/bytes/errs rx-pkts/bytes/errs\n");
 		goto out;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 06/29] l2tp: cleanup wonky alignment of line-broken function calls
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (4 preceding siblings ...)
  2020-07-21 17:31 ` [PATCH 05/29] l2tp: cleanup difficult-to-read line breaks Tom Parkin
@ 2020-07-21 17:31 ` Tom Parkin
  2020-07-21 17:31 ` [PATCH 07/29] l2tp: cleanup suspect code indent Tom Parkin
                   ` (23 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:31 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

Arguments should be aligned with the function call open parenthesis as
per checkpatch.  Tweak some function calls which were not aligned
correctly.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c    | 12 ++++++------
 net/l2tp/l2tp_debugfs.c |  2 +-
 net/l2tp/l2tp_ppp.c     |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 70891be75f77..a206ba97328f 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1283,10 +1283,10 @@ static void l2tp_tunnel_del_work(struct work_struct *work)
  * exit hook.
  */
 static int l2tp_tunnel_sock_create(struct net *net,
-				u32 tunnel_id,
-				u32 peer_tunnel_id,
-				struct l2tp_tunnel_cfg *cfg,
-				struct socket **sockp)
+				   u32 tunnel_id,
+				   u32 peer_tunnel_id,
+				   struct l2tp_tunnel_cfg *cfg,
+				   struct socket **sockp)
 {
 	int err = -EINVAL;
 	struct socket *sock = NULL;
@@ -1331,7 +1331,7 @@ static int l2tp_tunnel_sock_create(struct net *net,
 			struct sockaddr_l2tpip6 ip6_addr = {0};
 
 			err = sock_create_kern(net, AF_INET6, SOCK_DGRAM,
-					  IPPROTO_L2TP, &sock);
+					       IPPROTO_L2TP, &sock);
 			if (err < 0)
 				goto out;
 
@@ -1359,7 +1359,7 @@ static int l2tp_tunnel_sock_create(struct net *net,
 			struct sockaddr_l2tpip ip_addr = {0};
 
 			err = sock_create_kern(net, AF_INET, SOCK_DGRAM,
-					  IPPROTO_L2TP, &sock);
+					       IPPROTO_L2TP, &sock);
 			if (err < 0)
 				goto out;
 
diff --git a/net/l2tp/l2tp_debugfs.c b/net/l2tp/l2tp_debugfs.c
index 93181133e155..bea89c383b7d 100644
--- a/net/l2tp/l2tp_debugfs.c
+++ b/net/l2tp/l2tp_debugfs.c
@@ -145,7 +145,7 @@ static void l2tp_dfs_seq_tunnel_show(struct seq_file *m, void *v)
 			const struct ipv6_pinfo *np = inet6_sk(tunnel->sock);
 
 			seq_printf(m, " from %pI6c to %pI6c\n",
-				&np->saddr, &tunnel->sock->sk_v6_daddr);
+				   &np->saddr, &tunnel->sock->sk_v6_daddr);
 		} else
 #endif
 		seq_printf(m, " from %pI4 to %pI4\n",
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 3fed922addb5..f894dc275393 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -1638,7 +1638,7 @@ static __net_init int pppol2tp_init_net(struct net *net)
 	int err = 0;
 
 	pde = proc_create_net("pppol2tp", 0444, net->proc_net,
-			&pppol2tp_seq_ops, sizeof(struct pppol2tp_seq_data));
+			      &pppol2tp_seq_ops, sizeof(struct pppol2tp_seq_data));
 	if (!pde) {
 		err = -ENOMEM;
 		goto out;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 07/29] l2tp: cleanup suspect code indent
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (5 preceding siblings ...)
  2020-07-21 17:31 ` [PATCH 06/29] l2tp: cleanup wonky alignment of line-broken function calls Tom Parkin
@ 2020-07-21 17:31 ` Tom Parkin
  2020-07-21 17:32 ` [PATCH 08/29] l2tp: add identifier name in function pointer prototype Tom Parkin
                   ` (22 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:31 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

l2tp_core has conditionally compiled code in l2tp_xmit_skb for IPv6
support.  The structure of this code triggered a checkpatch warning
due to incorrect indentation.

Fix up the indentation to address the checkpatch warning.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index a206ba97328f..0b6649d6d97d 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1120,8 +1120,8 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len
 				      &sk->sk_v6_daddr, udp_len);
 		else
 #endif
-		udp_set_csum(sk->sk_no_check_tx, skb, inet->inet_saddr,
-			     inet->inet_daddr, udp_len);
+			udp_set_csum(sk->sk_no_check_tx, skb, inet->inet_saddr,
+				     inet->inet_daddr, udp_len);
 		break;
 
 	case L2TP_ENCAPTYPE_IP:
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 08/29] l2tp: add identifier name in function pointer prototype
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (6 preceding siblings ...)
  2020-07-21 17:31 ` [PATCH 07/29] l2tp: cleanup suspect code indent Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-21 17:32 ` [PATCH 09/29] l2tp: prefer using BIT macro Tom Parkin
                   ` (21 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

Reported by checkpatch:

        "WARNING: function definition argument 'struct sock *'
         should also have an identifier name"

Add an identifier name to help document the prototype.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 3ebb701eebbf..f23b3ff7ffff 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -154,7 +154,7 @@ struct l2tp_tunnel {
 	struct net		*l2tp_net;	/* the net we belong to */
 
 	refcount_t		ref_count;
-	void (*old_sk_destruct)(struct sock *);
+	void (*old_sk_destruct)(struct sock *sk);
 	struct sock		*sock;		/* parent socket */
 	int			fd;		/* parent fd, if tunnel socket was created
 						 * by userspace
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 09/29] l2tp: prefer using BIT macro
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (7 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 08/29] l2tp: add identifier name in function pointer prototype Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-21 17:32 ` [PATCH 10/29] l2tp: cleanup comparisons to NULL Tom Parkin
                   ` (20 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

Use BIT(x) rather than (1<<x), reported by checkpatch.pl.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index f23b3ff7ffff..2d2dd219a176 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -21,11 +21,11 @@
 
 /* Per tunnel, session hash table size */
 #define L2TP_HASH_BITS	4
-#define L2TP_HASH_SIZE	(1 << L2TP_HASH_BITS)
+#define L2TP_HASH_SIZE	BIT(L2TP_HASH_BITS)
 
 /* System-wide, session hash table size */
 #define L2TP_HASH_BITS_2	8
-#define L2TP_HASH_SIZE_2	(1 << L2TP_HASH_BITS_2)
+#define L2TP_HASH_SIZE_2	BIT(L2TP_HASH_BITS_2)
 
 struct sk_buff;
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 10/29] l2tp: cleanup comparisons to NULL
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (8 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 09/29] l2tp: prefer using BIT macro Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-21 17:32 ` [PATCH 11/29] l2tp: cleanup unnecessary braces in if statements Tom Parkin
                   ` (19 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

checkpatch warns about comparisons to NULL, e.g.

        CHECK: Comparison to NULL could be written "!rt"
        #474: FILE: net/l2tp/l2tp_ip.c:474:
        +       if (rt == NULL) {

These sort of comparisons are generally clearer and more readable
the way checkpatch suggests, so update l2tp accordingly.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c    | 20 ++++++++++----------
 net/l2tp/l2tp_debugfs.c | 12 ++++++------
 net/l2tp/l2tp_ip.c      |  2 +-
 net/l2tp/l2tp_ip6.c     |  2 +-
 net/l2tp/l2tp_netlink.c | 23 +++++++++++------------
 net/l2tp/l2tp_ppp.c     | 36 ++++++++++++++++++------------------
 6 files changed, 47 insertions(+), 48 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 0b6649d6d97d..dc8cb1fe94dc 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -410,7 +410,7 @@ static void l2tp_recv_dequeue_skb(struct l2tp_session *session, struct sk_buff *
 	}
 
 	/* call private receive handler */
-	if (session->recv_skb != NULL)
+	if (session->recv_skb)
 		(*session->recv_skb)(session, skb, L2TP_SKB_CB(skb)->length);
 	else
 		kfree_skb(skb);
@@ -909,7 +909,7 @@ int l2tp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 	struct l2tp_tunnel *tunnel;
 
 	tunnel = rcu_dereference_sk_user_data(sk);
-	if (tunnel == NULL)
+	if (!tunnel)
 		goto pass_up;
 
 	l2tp_dbg(tunnel, L2TP_MSG_DATA, "%s: received %d bytes\n",
@@ -1148,7 +1148,7 @@ static void l2tp_tunnel_destruct(struct sock *sk)
 {
 	struct l2tp_tunnel *tunnel = l2tp_tunnel(sk);
 
-	if (tunnel == NULL)
+	if (!tunnel)
 		goto end;
 
 	l2tp_info(tunnel, L2TP_MSG_CONTROL, "%s: closing...\n", tunnel->name);
@@ -1187,7 +1187,7 @@ static void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel)
 	struct hlist_node *tmp;
 	struct l2tp_session *session;
 
-	BUG_ON(tunnel == NULL);
+	BUG_ON(!tunnel);
 
 	l2tp_info(tunnel, L2TP_MSG_CONTROL, "%s: closing all sessions...\n",
 		  tunnel->name);
@@ -1212,7 +1212,7 @@ static void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel)
 			__l2tp_session_unhash(session);
 			l2tp_session_queue_purge(session);
 
-			if (session->session_close != NULL)
+			if (session->session_close)
 				(*session->session_close)(session);
 
 			l2tp_session_dec_refcount(session);
@@ -1404,11 +1404,11 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
 	int err;
 	enum l2tp_encap_type encap = L2TP_ENCAPTYPE_UDP;
 
-	if (cfg != NULL)
+	if (cfg)
 		encap = cfg->encap;
 
 	tunnel = kzalloc(sizeof(struct l2tp_tunnel), GFP_KERNEL);
-	if (tunnel == NULL) {
+	if (!tunnel) {
 		err = -ENOMEM;
 		goto err;
 	}
@@ -1423,7 +1423,7 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
 	rwlock_init(&tunnel->hlist_lock);
 	tunnel->acpt_newsess = true;
 
-	if (cfg != NULL)
+	if (cfg)
 		tunnel->debug = cfg->debug;
 
 	tunnel->encap = encap;
@@ -1612,7 +1612,7 @@ int l2tp_session_delete(struct l2tp_session *session)
 
 	__l2tp_session_unhash(session);
 	l2tp_session_queue_purge(session);
-	if (session->session_close != NULL)
+	if (session->session_close)
 		(*session->session_close)(session);
 
 	l2tp_session_dec_refcount(session);
@@ -1644,7 +1644,7 @@ struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunn
 	struct l2tp_session *session;
 
 	session = kzalloc(sizeof(struct l2tp_session) + priv_size, GFP_KERNEL);
-	if (session != NULL) {
+	if (session) {
 		session->magic = L2TP_SESSION_MAGIC;
 		session->tunnel = tunnel;
 
diff --git a/net/l2tp/l2tp_debugfs.c b/net/l2tp/l2tp_debugfs.c
index bea89c383b7d..70759acf2ae9 100644
--- a/net/l2tp/l2tp_debugfs.c
+++ b/net/l2tp/l2tp_debugfs.c
@@ -58,7 +58,7 @@ static void l2tp_dfs_next_session(struct l2tp_dfs_seq_data *pd)
 	pd->session = l2tp_session_get_nth(pd->tunnel, pd->session_idx);
 	pd->session_idx++;
 
-	if (pd->session == NULL) {
+	if (!pd->session) {
 		pd->session_idx = 0;
 		l2tp_dfs_next_tunnel(pd);
 	}
@@ -72,16 +72,16 @@ static void *l2tp_dfs_seq_start(struct seq_file *m, loff_t *offs)
 	if (!pos)
 		goto out;
 
-	BUG_ON(m->private == NULL);
+	BUG_ON(!m->private);
 	pd = m->private;
 
-	if (pd->tunnel == NULL)
+	if (!pd->tunnel)
 		l2tp_dfs_next_tunnel(pd);
 	else
 		l2tp_dfs_next_session(pd);
 
 	/* NULL tunnel and session indicates end of list */
-	if ((pd->tunnel == NULL) && (pd->session == NULL))
+	if (!pd->tunnel && !pd->session)
 		pd = NULL;
 
 out:
@@ -221,7 +221,7 @@ static void l2tp_dfs_seq_session_show(struct seq_file *m, void *v)
 		   atomic_long_read(&session->stats.rx_bytes),
 		   atomic_long_read(&session->stats.rx_errors));
 
-	if (session->show != NULL)
+	if (session->show)
 		session->show(m, session);
 }
 
@@ -268,7 +268,7 @@ static int l2tp_dfs_seq_open(struct inode *inode, struct file *file)
 	int rc = -ENOMEM;
 
 	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
-	if (pd == NULL)
+	if (!pd)
 		goto out;
 
 	/* Derive the network namespace from the pid opening the
diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
index 70f9fdaf6c86..d81564cf1e7f 100644
--- a/net/l2tp/l2tp_ip.c
+++ b/net/l2tp/l2tp_ip.c
@@ -471,7 +471,7 @@ static int l2tp_ip_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 		rt = (struct rtable *)__sk_dst_check(sk, 0);
 
 	rcu_read_lock();
-	if (rt == NULL) {
+	if (!rt) {
 		const struct ip_options_rcu *inet_opt;
 
 		inet_opt = rcu_dereference(inet->inet_opt);
diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index ca7696147c7e..614febf8dd0d 100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c
@@ -486,7 +486,7 @@ static int l2tp_ip6_push_pending_frames(struct sock *sk)
 	int err = 0;
 
 	skb = skb_peek(&sk->sk_write_queue);
-	if (skb == NULL)
+	if (!skb)
 		goto out;
 
 	transhdr = (__be32 *)skb_transport_header(skb);
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 0325ed2cfe8a..ac5769ef8e20 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -333,7 +333,7 @@ static int l2tp_nl_tunnel_send(struct sk_buff *skb, u32 portid, u32 seq, int fla
 		goto nla_put_failure;
 
 	nest = nla_nest_start_noflag(skb, L2TP_ATTR_STATS);
-	if (nest == NULL)
+	if (!nest)
 		goto nla_put_failure;
 
 	if (nla_put_u64_64bit(skb, L2TP_ATTR_TX_PACKETS,
@@ -475,7 +475,7 @@ static int l2tp_nl_cmd_tunnel_dump(struct sk_buff *skb, struct netlink_callback
 
 	for (;;) {
 		tunnel = l2tp_tunnel_get_nth(net, ti);
-		if (tunnel == NULL)
+		if (!tunnel)
 			goto out;
 
 		if (l2tp_nl_tunnel_send(skb, NETLINK_CB(cb->skb).portid,
@@ -598,14 +598,13 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
 		cfg.reorder_timeout = nla_get_msecs(info->attrs[L2TP_ATTR_RECV_TIMEOUT]);
 
 #ifdef CONFIG_MODULES
-	if (l2tp_nl_cmd_ops[cfg.pw_type] == NULL) {
+	if (!l2tp_nl_cmd_ops[cfg.pw_type]) {
 		genl_unlock();
 		request_module("net-l2tp-type-%u", cfg.pw_type);
 		genl_lock();
 	}
 #endif
-	if ((l2tp_nl_cmd_ops[cfg.pw_type] == NULL) ||
-	    (l2tp_nl_cmd_ops[cfg.pw_type]->session_create == NULL)) {
+	if (!l2tp_nl_cmd_ops[cfg.pw_type] || !l2tp_nl_cmd_ops[cfg.pw_type]->session_create) {
 		ret = -EPROTONOSUPPORT;
 		goto out_tunnel;
 	}
@@ -637,7 +636,7 @@ static int l2tp_nl_cmd_session_delete(struct sk_buff *skb, struct genl_info *inf
 	u16 pw_type;
 
 	session = l2tp_nl_session_get(info);
-	if (session == NULL) {
+	if (!session) {
 		ret = -ENODEV;
 		goto out;
 	}
@@ -662,7 +661,7 @@ static int l2tp_nl_cmd_session_modify(struct sk_buff *skb, struct genl_info *inf
 	struct l2tp_session *session;
 
 	session = l2tp_nl_session_get(info);
-	if (session == NULL) {
+	if (!session) {
 		ret = -ENODEV;
 		goto out;
 	}
@@ -729,7 +728,7 @@ static int l2tp_nl_session_send(struct sk_buff *skb, u32 portid, u32 seq, int fl
 		goto nla_put_failure;
 
 	nest = nla_nest_start_noflag(skb, L2TP_ATTR_STATS);
-	if (nest == NULL)
+	if (!nest)
 		goto nla_put_failure;
 
 	if (nla_put_u64_64bit(skb, L2TP_ATTR_TX_PACKETS,
@@ -774,7 +773,7 @@ static int l2tp_nl_cmd_session_get(struct sk_buff *skb, struct genl_info *info)
 	int ret;
 
 	session = l2tp_nl_session_get(info);
-	if (session == NULL) {
+	if (!session) {
 		ret = -ENODEV;
 		goto err;
 	}
@@ -813,14 +812,14 @@ static int l2tp_nl_cmd_session_dump(struct sk_buff *skb, struct netlink_callback
 	int si = cb->args[1];
 
 	for (;;) {
-		if (tunnel == NULL) {
+		if (!tunnel) {
 			tunnel = l2tp_tunnel_get_nth(net, ti);
-			if (tunnel == NULL)
+			if (!tunnel)
 				goto out;
 		}
 
 		session = l2tp_session_get_nth(tunnel, si);
-		if (session == NULL) {
+		if (!session) {
 			ti++;
 			l2tp_tunnel_dec_refcount(tunnel);
 			tunnel = NULL;
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index f894dc275393..7404661d4117 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -154,12 +154,12 @@ static inline struct l2tp_session *pppol2tp_sock_to_session(struct sock *sk)
 {
 	struct l2tp_session *session;
 
-	if (sk == NULL)
+	if (!sk)
 		return NULL;
 
 	sock_hold(sk);
 	session = (struct l2tp_session *)(sk->sk_user_data);
-	if (session == NULL) {
+	if (!session) {
 		sock_put(sk);
 		goto out;
 	}
@@ -217,7 +217,7 @@ static void pppol2tp_recv(struct l2tp_session *session, struct sk_buff *skb, int
 	 */
 	rcu_read_lock();
 	sk = rcu_dereference(ps->sk);
-	if (sk == NULL)
+	if (!sk)
 		goto no_sock;
 
 	/* If the first two bytes are 0xFF03, consider that it is the PPP's
@@ -285,7 +285,7 @@ static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m,
 	/* Get session and tunnel contexts */
 	error = -EBADF;
 	session = pppol2tp_sock_to_session(sk);
-	if (session == NULL)
+	if (!session)
 		goto error;
 
 	tunnel = session->tunnel;
@@ -360,7 +360,7 @@ static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
 
 	/* Get session and tunnel contexts from the socket */
 	session = pppol2tp_sock_to_session(sk);
-	if (session == NULL)
+	if (!session)
 		goto abort;
 
 	tunnel = session->tunnel;
@@ -703,7 +703,7 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
 	 * tunnel id.
 	 */
 	if (!info.session_id && !info.peer_session_id) {
-		if (tunnel == NULL) {
+		if (!tunnel) {
 			struct l2tp_tunnel_cfg tcfg = {
 				.encap = L2TP_ENCAPTYPE_UDP,
 				.debug = 0,
@@ -738,11 +738,11 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
 	} else {
 		/* Error if we can't find the tunnel */
 		error = -ENOENT;
-		if (tunnel == NULL)
+		if (!tunnel)
 			goto end;
 
 		/* Error if socket is not prepped */
-		if (tunnel->sock == NULL)
+		if (!tunnel->sock)
 			goto end;
 	}
 
@@ -911,14 +911,14 @@ static int pppol2tp_getname(struct socket *sock, struct sockaddr *uaddr,
 	struct pppol2tp_session *pls;
 
 	error = -ENOTCONN;
-	if (sk == NULL)
+	if (!sk)
 		goto end;
 	if (!(sk->sk_state & PPPOX_CONNECTED))
 		goto end;
 
 	error = -EBADF;
 	session = pppol2tp_sock_to_session(sk);
-	if (session == NULL)
+	if (!session)
 		goto end;
 
 	pls = l2tp_session_priv(session);
@@ -1263,13 +1263,13 @@ static int pppol2tp_setsockopt(struct socket *sock, int level, int optname,
 		return -EFAULT;
 
 	err = -ENOTCONN;
-	if (sk->sk_user_data == NULL)
+	if (!sk->sk_user_data)
 		goto end;
 
 	/* Get session context from the socket */
 	err = -EBADF;
 	session = pppol2tp_sock_to_session(sk);
-	if (session == NULL)
+	if (!session)
 		goto end;
 
 	/* Special case: if session_id == 0x0000, treat as operation on tunnel
@@ -1382,13 +1382,13 @@ static int pppol2tp_getsockopt(struct socket *sock, int level, int optname,
 		return -EINVAL;
 
 	err = -ENOTCONN;
-	if (sk->sk_user_data == NULL)
+	if (!sk->sk_user_data)
 		goto end;
 
 	/* Get the session context */
 	err = -EBADF;
 	session = pppol2tp_sock_to_session(sk);
-	if (session == NULL)
+	if (!session)
 		goto end;
 
 	/* Special case: if session_id == 0x0000, treat as operation on tunnel */
@@ -1464,7 +1464,7 @@ static void pppol2tp_next_session(struct net *net, struct pppol2tp_seq_data *pd)
 	pd->session = l2tp_session_get_nth(pd->tunnel, pd->session_idx);
 	pd->session_idx++;
 
-	if (pd->session == NULL) {
+	if (!pd->session) {
 		pd->session_idx = 0;
 		pppol2tp_next_tunnel(net, pd);
 	}
@@ -1479,17 +1479,17 @@ static void *pppol2tp_seq_start(struct seq_file *m, loff_t *offs)
 	if (!pos)
 		goto out;
 
-	BUG_ON(m->private == NULL);
+	BUG_ON(!m->private);
 	pd = m->private;
 	net = seq_file_net(m);
 
-	if (pd->tunnel == NULL)
+	if (!pd->tunnel)
 		pppol2tp_next_tunnel(net, pd);
 	else
 		pppol2tp_next_session(net, pd);
 
 	/* NULL tunnel and session indicates end of list */
-	if ((pd->tunnel == NULL) && (pd->session == NULL))
+	if (!pd->tunnel && !pd->session)
 		pd = NULL;
 
 out:
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 11/29] l2tp: cleanup unnecessary braces in if statements
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (9 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 10/29] l2tp: cleanup comparisons to NULL Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-21 17:32 ` [PATCH 12/29] l2tp: prefer seq_puts for unformatted output Tom Parkin
                   ` (18 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

These checks are all simple and don't benefit from extra braces to
clarify intent.  Remove them for easier-reading code.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c |  6 +++---
 net/l2tp/l2tp_ppp.c  | 23 +++++++++--------------
 2 files changed, 12 insertions(+), 17 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index dc8cb1fe94dc..acba7757bd79 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -681,7 +681,7 @@ void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb,
 		 * check if we sre sending sequence numbers and if not,
 		 * configure it so.
 		 */
-		if ((!session->lns_mode) && (!session->send_seq)) {
+		if (!session->lns_mode && !session->send_seq) {
 			l2tp_info(session, L2TP_MSG_SEQ,
 				  "%s: requested to enable seq numbers by LNS\n",
 				  session->name);
@@ -705,7 +705,7 @@ void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb,
 		 * If we're the LNS and we're sending sequence numbers, the
 		 * LAC is broken. Discard the frame.
 		 */
-		if ((!session->lns_mode) && (session->send_seq)) {
+		if (!session->lns_mode && session->send_seq) {
 			l2tp_info(session, L2TP_MSG_SEQ,
 				  "%s: requested to disable seq numbers by LNS\n",
 				  session->name);
@@ -1387,7 +1387,7 @@ static int l2tp_tunnel_sock_create(struct net *net,
 
 out:
 	*sockp = sock;
-	if ((err < 0) && sock) {
+	if (err < 0 && sock) {
 		kernel_sock_shutdown(sock, SHUT_RDWR);
 		sock_release(sock);
 		*sockp = NULL;
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 7404661d4117..e58fe7e3b884 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -802,8 +802,7 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
 	 * the internal context for use by ioctl() and sockopt()
 	 * handlers.
 	 */
-	if ((session->session_id == 0) &&
-	    (session->peer_session_id == 0)) {
+	if (session->session_id == 0 && session->peer_session_id == 0) {
 		error = 0;
 		goto out_no_ppp;
 	}
@@ -925,7 +924,7 @@ static int pppol2tp_getname(struct socket *sock, struct sockaddr *uaddr,
 	tunnel = session->tunnel;
 
 	inet = inet_sk(tunnel->sock);
-	if ((tunnel->version == 2) && (tunnel->sock->sk_family == AF_INET)) {
+	if (tunnel->version == 2 && tunnel->sock->sk_family == AF_INET) {
 		struct sockaddr_pppol2tp sp;
 
 		len = sizeof(sp);
@@ -943,8 +942,7 @@ static int pppol2tp_getname(struct socket *sock, struct sockaddr *uaddr,
 		sp.pppol2tp.addr.sin_addr.s_addr = inet->inet_daddr;
 		memcpy(uaddr, &sp, len);
 #if IS_ENABLED(CONFIG_IPV6)
-	} else if ((tunnel->version == 2) &&
-		   (tunnel->sock->sk_family == AF_INET6)) {
+	} else if (tunnel->version == 2 && tunnel->sock->sk_family == AF_INET6) {
 		struct sockaddr_pppol2tpin6 sp;
 
 		len = sizeof(sp);
@@ -962,8 +960,7 @@ static int pppol2tp_getname(struct socket *sock, struct sockaddr *uaddr,
 		memcpy(&sp.pppol2tp.addr.sin6_addr, &tunnel->sock->sk_v6_daddr,
 		       sizeof(tunnel->sock->sk_v6_daddr));
 		memcpy(uaddr, &sp, len);
-	} else if ((tunnel->version == 3) &&
-		   (tunnel->sock->sk_family == AF_INET6)) {
+	} else if (tunnel->version == 3 && tunnel->sock->sk_family == AF_INET6) {
 		struct sockaddr_pppol2tpv3in6 sp;
 
 		len = sizeof(sp);
@@ -1179,7 +1176,7 @@ static int pppol2tp_session_setsockopt(struct sock *sk,
 
 	switch (optname) {
 	case PPPOL2TP_SO_RECVSEQ:
-		if ((val != 0) && (val != 1)) {
+		if (val != 0 && val != 1) {
 			err = -EINVAL;
 			break;
 		}
@@ -1190,7 +1187,7 @@ static int pppol2tp_session_setsockopt(struct sock *sk,
 		break;
 
 	case PPPOL2TP_SO_SENDSEQ:
-		if ((val != 0) && (val != 1)) {
+		if (val != 0 && val != 1) {
 			err = -EINVAL;
 			break;
 		}
@@ -1208,7 +1205,7 @@ static int pppol2tp_session_setsockopt(struct sock *sk,
 		break;
 
 	case PPPOL2TP_SO_LNSMODE:
-		if ((val != 0) && (val != 1)) {
+		if (val != 0 && val != 1) {
 			err = -EINVAL;
 			break;
 		}
@@ -1274,8 +1271,7 @@ static int pppol2tp_setsockopt(struct socket *sock, int level, int optname,
 
 	/* Special case: if session_id == 0x0000, treat as operation on tunnel
 	 */
-	if ((session->session_id == 0) &&
-	    (session->peer_session_id == 0)) {
+	if (session->session_id == 0 && session->peer_session_id == 0) {
 		tunnel = session->tunnel;
 		err = pppol2tp_tunnel_setsockopt(sk, tunnel, optname, val);
 	} else {
@@ -1392,8 +1388,7 @@ static int pppol2tp_getsockopt(struct socket *sock, int level, int optname,
 		goto end;
 
 	/* Special case: if session_id == 0x0000, treat as operation on tunnel */
-	if ((session->session_id == 0) &&
-	    (session->peer_session_id == 0)) {
+	if (session->session_id == 0 && session->peer_session_id == 0) {
 		tunnel = session->tunnel;
 		err = pppol2tp_tunnel_getsockopt(sk, tunnel, optname, &val);
 		if (err)
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 12/29] l2tp: prefer seq_puts for unformatted output
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (10 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 11/29] l2tp: cleanup unnecessary braces in if statements Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-21 17:32 ` [PATCH 13/29] l2tp: avoid multiple assignments Tom Parkin
                   ` (17 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

checkpatch warns about use of seq_printf where seq_puts would do.

Modify l2tp_debugfs accordingly.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_debugfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/l2tp/l2tp_debugfs.c b/net/l2tp/l2tp_debugfs.c
index 70759acf2ae9..117a6697da72 100644
--- a/net/l2tp/l2tp_debugfs.c
+++ b/net/l2tp/l2tp_debugfs.c
@@ -199,7 +199,7 @@ static void l2tp_dfs_seq_session_show(struct seq_file *m, void *v)
 			seq_printf(m, "%02x%02x%02x%02x",
 				   session->cookie[4], session->cookie[5],
 				   session->cookie[6], session->cookie[7]);
-		seq_printf(m, "\n");
+		seq_puts(m, "\n");
 	}
 	if (session->peer_cookie_len) {
 		seq_printf(m, "   peer cookie %02x%02x%02x%02x",
@@ -209,7 +209,7 @@ static void l2tp_dfs_seq_session_show(struct seq_file *m, void *v)
 			seq_printf(m, "%02x%02x%02x%02x",
 				   session->peer_cookie[4], session->peer_cookie[5],
 				   session->peer_cookie[6], session->peer_cookie[7]);
-		seq_printf(m, "\n");
+		seq_puts(m, "\n");
 	}
 
 	seq_printf(m, "   %hu/%hu tx %ld/%ld/%ld rx %ld/%ld/%ld\n",
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 13/29] l2tp: avoid multiple assignments
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (11 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 12/29] l2tp: prefer seq_puts for unformatted output Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-21 17:32 ` [PATCH 14/29] l2tp: line-break long function prototypes Tom Parkin
                   ` (16 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

checkpatch warns about multiple assignments.

Update l2tp accordingly.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c |  6 +++---
 net/l2tp/l2tp_ip.c   | 12 ++++++++----
 net/l2tp/l2tp_ip6.c  |  6 ++++--
 3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index acba7757bd79..7218925edbf2 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -619,8 +619,8 @@ void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb,
 		      int length)
 {
 	struct l2tp_tunnel *tunnel = session->tunnel;
+	u32 ns = 0, nr = 0;
 	int offset;
-	u32 ns, nr;
 
 	/* Parse and check optional cookie */
 	if (session->peer_cookie_len > 0) {
@@ -642,7 +642,6 @@ void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb,
 	 * the control of the LNS.  If no sequence numbers present but
 	 * we were expecting them, discard frame.
 	 */
-	ns = nr = 0;
 	L2TP_SKB_CB(skb)->has_seq = 0;
 	if (tunnel->version == L2TP_HDR_VER_2) {
 		if (hdrflags & L2TP_HDRFLAG_S) {
@@ -824,7 +823,8 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
 	}
 
 	/* Point to L2TP header */
-	optr = ptr = skb->data;
+	optr = skb->data;
+	ptr = skb->data;
 
 	/* Get L2TP header flags */
 	hdrflags = ntohs(*(__be16 *)ptr);
diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
index d81564cf1e7f..a159cb2bf0f4 100644
--- a/net/l2tp/l2tp_ip.c
+++ b/net/l2tp/l2tp_ip.c
@@ -124,7 +124,8 @@ static int l2tp_ip_recv(struct sk_buff *skb)
 		goto discard;
 
 	/* Point to L2TP header */
-	optr = ptr = skb->data;
+	optr = skb->data;
+	ptr = skb->data;
 	session_id = ntohl(*((__be32 *)ptr));
 	ptr += 4;
 
@@ -153,7 +154,8 @@ static int l2tp_ip_recv(struct sk_buff *skb)
 			goto discard_sess;
 
 		/* Point to L2TP header */
-		optr = ptr = skb->data;
+		optr = skb->data;
+		ptr = skb->data;
 		ptr += 4;
 		pr_debug("%s: ip recv\n", tunnel->name);
 		print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, ptr, length);
@@ -284,8 +286,10 @@ static int l2tp_ip_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 	    chk_addr_ret != RTN_MULTICAST && chk_addr_ret != RTN_BROADCAST)
 		goto out;
 
-	if (addr->l2tp_addr.s_addr)
-		inet->inet_rcv_saddr = inet->inet_saddr = addr->l2tp_addr.s_addr;
+	if (addr->l2tp_addr.s_addr) {
+		inet->inet_rcv_saddr = addr->l2tp_addr.s_addr;
+		inet->inet_saddr = addr->l2tp_addr.s_addr;
+	}
 	if (chk_addr_ret == RTN_MULTICAST || chk_addr_ret == RTN_BROADCAST)
 		inet->inet_saddr = 0;  /* Use device */
 
diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index 614febf8dd0d..bc757bc7e264 100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c
@@ -137,7 +137,8 @@ static int l2tp_ip6_recv(struct sk_buff *skb)
 		goto discard;
 
 	/* Point to L2TP header */
-	optr = ptr = skb->data;
+	optr = skb->data;
+	ptr = skb->data;
 	session_id = ntohl(*((__be32 *)ptr));
 	ptr += 4;
 
@@ -166,7 +167,8 @@ static int l2tp_ip6_recv(struct sk_buff *skb)
 			goto discard_sess;
 
 		/* Point to L2TP header */
-		optr = ptr = skb->data;
+		optr = skb->data;
+		ptr = skb->data;
 		ptr += 4;
 		pr_debug("%s: ip recv\n", tunnel->name);
 		print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, ptr, length);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 14/29] l2tp: line-break long function prototypes
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (12 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 13/29] l2tp: avoid multiple assignments Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-21 17:32 ` [PATCH 15/29] l2tp: comment per net spinlock instances Tom Parkin
                   ` (15 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

In l2tp_core.c both l2tp_tunnel_create and l2tp_session_create take
quite a number of arguments and have a correspondingly long prototype.

This is both quite difficult to scan visually, and triggers checkpatch
warnings.

Add a line break to make these function prototypes more readable.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 7218925edbf2..ef393604e66e 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1398,7 +1398,8 @@ static int l2tp_tunnel_sock_create(struct net *net,
 
 static struct lock_class_key l2tp_socket_class;
 
-int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32 peer_tunnel_id, struct l2tp_tunnel_cfg *cfg, struct l2tp_tunnel **tunnelp)
+int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32 peer_tunnel_id,
+		       struct l2tp_tunnel_cfg *cfg, struct l2tp_tunnel **tunnelp)
 {
 	struct l2tp_tunnel *tunnel = NULL;
 	int err;
@@ -1639,7 +1640,8 @@ void l2tp_session_set_header_len(struct l2tp_session *session, int version)
 }
 EXPORT_SYMBOL_GPL(l2tp_session_set_header_len);
 
-struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunnel, u32 session_id, u32 peer_session_id, struct l2tp_session_cfg *cfg)
+struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunnel, u32 session_id,
+					 u32 peer_session_id, struct l2tp_session_cfg *cfg)
 {
 	struct l2tp_session *session;
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 15/29] l2tp: comment per net spinlock instances
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (13 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 14/29] l2tp: line-break long function prototypes Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-21 17:32 ` [PATCH 16/29] l2tp: fix up incorrect comment in l2tp_recv_common Tom Parkin
                   ` (14 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

checkpatch warns on spinlocks which are not commented as to their use.

Comment l2tp's per net locks accordingly.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index ef393604e66e..182386ad20e2 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -101,8 +101,10 @@ static struct workqueue_struct *l2tp_wq;
 static unsigned int l2tp_net_id;
 struct l2tp_net {
 	struct list_head l2tp_tunnel_list;
+	/* Lock for write access to l2tp_tunnel_list */
 	spinlock_t l2tp_tunnel_list_lock;
 	struct hlist_head l2tp_session_hlist[L2TP_HASH_SIZE_2];
+	/* Lock for write access to l2tp_session_hlist */
 	spinlock_t l2tp_session_hlist_lock;
 };
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 16/29] l2tp: fix up incorrect comment in l2tp_recv_common
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (14 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 15/29] l2tp: comment per net spinlock instances Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-21 17:32 ` [PATCH 17/29] l2tp: avoid precidence issues in L2TP_SKB_CB macro Tom Parkin
                   ` (13 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

RFC2661 section 5.4 states that:

"The LNS controls enabling and disabling of sequence numbers by sending a
data message with or without sequence numbers present at any time during
the life of a session."

l2tp handles this correctly in l2tp_recv_common, but the comment around
the code was incorrect and confusing.  Fix up the comment accordingly.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 182386ad20e2..177accb01993 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -678,7 +678,7 @@ void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb,
 	}
 
 	if (L2TP_SKB_CB(skb)->has_seq) {
-		/* Received a packet with sequence numbers. If we're the LNS,
+		/* Received a packet with sequence numbers. If we're the LAC,
 		 * check if we sre sending sequence numbers and if not,
 		 * configure it so.
 		 */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 17/29] l2tp: avoid precidence issues in L2TP_SKB_CB macro
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (15 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 16/29] l2tp: fix up incorrect comment in l2tp_recv_common Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-28 16:21   ` Joe Perches
  2020-07-21 17:32 ` [PATCH 18/29] l2tp: WARN_ON rather than BUG_ON in l2tp_debugfs.c Tom Parkin
                   ` (12 subsequent siblings)
  29 siblings, 1 reply; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

checkpatch warned about the L2TP_SKB_CB macro's use of its argument: add
braces to avoid the problem.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 177accb01993..4973a0f035e3 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -93,7 +93,7 @@ struct l2tp_skb_cb {
 	unsigned long		expires;
 };
 
-#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *)&skb->cb[sizeof(struct inet_skb_parm)])
+#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *)&(skb)->cb[sizeof(struct inet_skb_parm)])
 
 static struct workqueue_struct *l2tp_wq;
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 18/29] l2tp: WARN_ON rather than BUG_ON in l2tp_debugfs.c
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (16 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 17/29] l2tp: avoid precidence issues in L2TP_SKB_CB macro Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-21 17:32 ` [PATCH 19/29] l2tp: use a function to render tunnel address in l2tp_debugfs Tom Parkin
                   ` (11 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

l2tp_dfs_seq_start had a BUG_ON to catch a possible programming error in
l2tp_dfs_seq_open.

Since we can easily bail out of l2tp_dfs_seq_start, prefer to do that
and flag the error with a WARN_ON rather than crashing the kernel.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_debugfs.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/l2tp/l2tp_debugfs.c b/net/l2tp/l2tp_debugfs.c
index 117a6697da72..800a17b988be 100644
--- a/net/l2tp/l2tp_debugfs.c
+++ b/net/l2tp/l2tp_debugfs.c
@@ -72,7 +72,14 @@ static void *l2tp_dfs_seq_start(struct seq_file *m, loff_t *offs)
 	if (!pos)
 		goto out;
 
-	BUG_ON(!m->private);
+	/* Unexpected: m->private is set in l2tp_dfs_seq_open.
+	 * Warn on this and bail out early.
+	 */
+	if (!m->private) {
+		WARN_ON(!m->private);
+		pd = NULL;
+		goto out;
+	}
 	pd = m->private;
 
 	if (!pd->tunnel)
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 19/29] l2tp: use a function to render tunnel address in l2tp_debugfs
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (17 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 18/29] l2tp: WARN_ON rather than BUG_ON in l2tp_debugfs.c Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-21 17:32 ` [PATCH 20/29] l2tp: cleanup netlink send of tunnel address information Tom Parkin
                   ` (10 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

The L2TP tunnel socket address may be IPv4 or IPv6.

The conditionally compiled code for IPv6 support confused checkpatch's
indentation and brace checking.

To avoid the warning, and make the code somewhat easier to read, split
the address rendering out into a function where the conditional
compilation can be handled more cleanly.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_debugfs.c | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/net/l2tp/l2tp_debugfs.c b/net/l2tp/l2tp_debugfs.c
index 800a17b988be..d8cddc82da4b 100644
--- a/net/l2tp/l2tp_debugfs.c
+++ b/net/l2tp/l2tp_debugfs.c
@@ -121,6 +121,25 @@ static void l2tp_dfs_seq_stop(struct seq_file *p, void *v)
 	}
 }
 
+static void l2tp_dfs_seq_tunnel_show_addr(struct seq_file *m, struct l2tp_tunnel *tunnel)
+{
+	struct inet_sock *inet = inet_sk(tunnel->sock);
+
+#if IS_ENABLED(CONFIG_IPV6)
+	if (tunnel->sock->sk_family == AF_INET6) {
+		const struct ipv6_pinfo *np = inet6_sk(tunnel->sock);
+
+		seq_printf(m, " from %pI6c to %pI6c\n", &np->saddr, &tunnel->sock->sk_v6_daddr);
+	}
+#endif
+	if (tunnel->sock->sk_family == AF_INET)
+		seq_printf(m, " from %pI4 to %pI4\n", &inet->inet_saddr, &inet->inet_daddr);
+
+	if (tunnel->encap == L2TP_ENCAPTYPE_UDP)
+		seq_printf(m, " source port %hu, dest port %hu\n",
+			   ntohs(inet->inet_sport), ntohs(inet->inet_dport));
+}
+
 static void l2tp_dfs_seq_tunnel_show(struct seq_file *m, void *v)
 {
 	struct l2tp_tunnel *tunnel = v;
@@ -144,23 +163,8 @@ static void l2tp_dfs_seq_tunnel_show(struct seq_file *m, void *v)
 	read_unlock_bh(&tunnel->hlist_lock);
 
 	seq_printf(m, "\nTUNNEL %u peer %u", tunnel->tunnel_id, tunnel->peer_tunnel_id);
-	if (tunnel->sock) {
-		struct inet_sock *inet = inet_sk(tunnel->sock);
-
-#if IS_ENABLED(CONFIG_IPV6)
-		if (tunnel->sock->sk_family == AF_INET6) {
-			const struct ipv6_pinfo *np = inet6_sk(tunnel->sock);
-
-			seq_printf(m, " from %pI6c to %pI6c\n",
-				   &np->saddr, &tunnel->sock->sk_v6_daddr);
-		} else
-#endif
-		seq_printf(m, " from %pI4 to %pI4\n",
-			   &inet->inet_saddr, &inet->inet_daddr);
-		if (tunnel->encap == L2TP_ENCAPTYPE_UDP)
-			seq_printf(m, " source port %hu, dest port %hu\n",
-				   ntohs(inet->inet_sport), ntohs(inet->inet_dport));
-	}
+	if (tunnel->sock)
+		l2tp_dfs_seq_tunnel_show_addr(m, tunnel);
 	seq_printf(m, " L2TPv%d, %s\n", tunnel->version,
 		   tunnel->encap == L2TP_ENCAPTYPE_UDP ? "UDP" :
 		   tunnel->encap == L2TP_ENCAPTYPE_IP ? "IP" :
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 20/29] l2tp: cleanup netlink send of tunnel address information
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (18 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 19/29] l2tp: use a function to render tunnel address in l2tp_debugfs Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-21 17:32 ` [PATCH 21/29] l2tp: cleanup netlink tunnel create address handling Tom Parkin
                   ` (9 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

l2tp_nl_tunnel_send has conditionally compiled code to support AF_INET6,
which makes the code difficult to follow and triggers checkpatch
warnings.

Split the code out into functions to handle the AF_INET v.s. AF_INET6
cases, which both improves readability and resolves the checkpatch
warnings.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_netlink.c | 126 ++++++++++++++++++++++------------------
 1 file changed, 70 insertions(+), 56 deletions(-)

diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index ac5769ef8e20..8e03f2e367a0 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -310,16 +310,79 @@ static int l2tp_nl_cmd_tunnel_modify(struct sk_buff *skb, struct genl_info *info
 	return ret;
 }
 
+#if IS_ENABLED(CONFIG_IPV6)
+static int l2tp_nl_tunnel_send_addr6(struct sk_buff *skb, struct sock *sk,
+				     enum l2tp_encap_type encap)
+{
+	struct inet_sock *inet = inet_sk(sk);
+	struct ipv6_pinfo *np = inet6_sk(sk);
+
+	switch (encap) {
+	case L2TP_ENCAPTYPE_UDP:
+		if (udp_get_no_check6_tx(sk) &&
+		    nla_put_flag(skb, L2TP_ATTR_UDP_ZERO_CSUM6_TX))
+			return -1;
+		if (udp_get_no_check6_rx(sk) &&
+		    nla_put_flag(skb, L2TP_ATTR_UDP_ZERO_CSUM6_RX))
+			return -1;
+		if (nla_put_u16(skb, L2TP_ATTR_UDP_SPORT, ntohs(inet->inet_sport)) ||
+		    nla_put_u16(skb, L2TP_ATTR_UDP_DPORT, ntohs(inet->inet_dport)))
+			return -1;
+		fallthrough;
+	case L2TP_ENCAPTYPE_IP:
+		if (nla_put_in6_addr(skb, L2TP_ATTR_IP6_SADDR, &np->saddr) ||
+		    nla_put_in6_addr(skb, L2TP_ATTR_IP6_DADDR, &sk->sk_v6_daddr))
+			return -1;
+		break;
+	}
+	return 0;
+}
+#endif
+
+static int l2tp_nl_tunnel_send_addr4(struct sk_buff *skb, struct sock *sk,
+				     enum l2tp_encap_type encap)
+{
+	struct inet_sock *inet = inet_sk(sk);
+
+	switch (encap) {
+	case L2TP_ENCAPTYPE_UDP:
+		if (nla_put_u8(skb, L2TP_ATTR_UDP_CSUM, !sk->sk_no_check_tx) ||
+		    nla_put_u16(skb, L2TP_ATTR_UDP_SPORT, ntohs(inet->inet_sport)) ||
+		    nla_put_u16(skb, L2TP_ATTR_UDP_DPORT, ntohs(inet->inet_dport)))
+			return -1;
+		fallthrough;
+	case L2TP_ENCAPTYPE_IP:
+		if (nla_put_in_addr(skb, L2TP_ATTR_IP_SADDR, inet->inet_saddr) ||
+		    nla_put_in_addr(skb, L2TP_ATTR_IP_DADDR, inet->inet_daddr))
+			return -1;
+		break;
+	}
+
+	return 0;
+}
+
+/* Append attributes for the tunnel address, handling the different attribute types
+ * used for different tunnel encapsulation and AF_INET v.s. AF_INET6.
+ */
+static int l2tp_nl_tunnel_send_addr(struct sk_buff *skb, struct l2tp_tunnel *tunnel)
+{
+	struct sock *sk = tunnel->sock;
+
+	if (!sk)
+		return 0;
+
+#if IS_ENABLED(CONFIG_IPV6)
+	if (sk->sk_family == AF_INET6)
+		return l2tp_nl_tunnel_send_addr6(skb, sk, tunnel->encap);
+#endif
+	return l2tp_nl_tunnel_send_addr4(skb, sk, tunnel->encap);
+}
+
 static int l2tp_nl_tunnel_send(struct sk_buff *skb, u32 portid, u32 seq, int flags,
 			       struct l2tp_tunnel *tunnel, u8 cmd)
 {
 	void *hdr;
 	struct nlattr *nest;
-	struct sock *sk = NULL;
-	struct inet_sock *inet;
-#if IS_ENABLED(CONFIG_IPV6)
-	struct ipv6_pinfo *np = NULL;
-#endif
 
 	hdr = genlmsg_put(skb, portid, seq, &l2tp_nl_family, flags, cmd);
 	if (!hdr)
@@ -363,58 +426,9 @@ static int l2tp_nl_tunnel_send(struct sk_buff *skb, u32 portid, u32 seq, int fla
 		goto nla_put_failure;
 	nla_nest_end(skb, nest);
 
-	sk = tunnel->sock;
-	if (!sk)
-		goto out;
-
-#if IS_ENABLED(CONFIG_IPV6)
-	if (sk->sk_family == AF_INET6)
-		np = inet6_sk(sk);
-#endif
-
-	inet = inet_sk(sk);
-
-	switch (tunnel->encap) {
-	case L2TP_ENCAPTYPE_UDP:
-		switch (sk->sk_family) {
-		case AF_INET:
-			if (nla_put_u8(skb, L2TP_ATTR_UDP_CSUM, !sk->sk_no_check_tx))
-				goto nla_put_failure;
-			break;
-#if IS_ENABLED(CONFIG_IPV6)
-		case AF_INET6:
-			if (udp_get_no_check6_tx(sk) &&
-			    nla_put_flag(skb, L2TP_ATTR_UDP_ZERO_CSUM6_TX))
-				goto nla_put_failure;
-			if (udp_get_no_check6_rx(sk) &&
-			    nla_put_flag(skb, L2TP_ATTR_UDP_ZERO_CSUM6_RX))
-				goto nla_put_failure;
-			break;
-#endif
-		}
-		if (nla_put_u16(skb, L2TP_ATTR_UDP_SPORT, ntohs(inet->inet_sport)) ||
-		    nla_put_u16(skb, L2TP_ATTR_UDP_DPORT, ntohs(inet->inet_dport)))
-			goto nla_put_failure;
-		/* fall through  */
-	case L2TP_ENCAPTYPE_IP:
-#if IS_ENABLED(CONFIG_IPV6)
-		if (np) {
-			if (nla_put_in6_addr(skb, L2TP_ATTR_IP6_SADDR,
-					     &np->saddr) ||
-			    nla_put_in6_addr(skb, L2TP_ATTR_IP6_DADDR,
-					     &sk->sk_v6_daddr))
-				goto nla_put_failure;
-		} else
-#endif
-		if (nla_put_in_addr(skb, L2TP_ATTR_IP_SADDR,
-				    inet->inet_saddr) ||
-		    nla_put_in_addr(skb, L2TP_ATTR_IP_DADDR,
-				    inet->inet_daddr))
-			goto nla_put_failure;
-		break;
-	}
+	if (l2tp_nl_tunnel_send_addr(skb, tunnel))
+		goto nla_put_failure;
 
-out:
 	genlmsg_end(skb, hdr);
 	return 0;
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 21/29] l2tp: cleanup netlink tunnel create address handling
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (19 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 20/29] l2tp: cleanup netlink send of tunnel address information Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-21 17:32 ` [PATCH 22/29] l2tp: cleanup kzalloc calls Tom Parkin
                   ` (8 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

When creating an L2TP tunnel using the netlink API, userspace must
either pass a socket FD for the tunnel to use (for managed tunnels),
or specify the tunnel source/destination address (for unmanaged
tunnels).

Since source/destination addresses may be AF_INET or AF_INET6, the l2tp
netlink code has conditionally compiled blocks to support IPv6.

Rather than embedding these directly into l2tp_nl_cmd_tunnel_create
(where it makes the code difficult to read and confuses checkpatch to
boot) split the handling of address-related attributes into a separate
function.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_netlink.c | 57 ++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 24 deletions(-)

diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 8e03f2e367a0..35716a6e1e2c 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -155,12 +155,38 @@ static int l2tp_session_notify(struct genl_family *family,
 	return ret;
 }
 
+static int l2tp_nl_cmd_tunnel_create_get_addr(struct nlattr **attrs, struct l2tp_tunnel_cfg *cfg)
+{
+	if (attrs[L2TP_ATTR_UDP_SPORT])
+		cfg->local_udp_port = nla_get_u16(attrs[L2TP_ATTR_UDP_SPORT]);
+	if (attrs[L2TP_ATTR_UDP_DPORT])
+		cfg->peer_udp_port = nla_get_u16(attrs[L2TP_ATTR_UDP_DPORT]);
+	cfg->use_udp_checksums = nla_get_flag(attrs[L2TP_ATTR_UDP_CSUM]);
+
+	/* Must have either AF_INET or AF_INET6 address for source and destination */
+#if IS_ENABLED(CONFIG_IPV6)
+	if (attrs[L2TP_ATTR_IP6_SADDR] && attrs[L2TP_ATTR_IP6_DADDR]) {
+		cfg->local_ip6 = nla_data(attrs[L2TP_ATTR_IP6_SADDR]);
+		cfg->peer_ip6 = nla_data(attrs[L2TP_ATTR_IP6_DADDR]);
+		cfg->udp6_zero_tx_checksums = nla_get_flag(attrs[L2TP_ATTR_UDP_ZERO_CSUM6_TX]);
+		cfg->udp6_zero_rx_checksums = nla_get_flag(attrs[L2TP_ATTR_UDP_ZERO_CSUM6_RX]);
+		return 0;
+	}
+#endif
+	if (attrs[L2TP_ATTR_IP_SADDR] && attrs[L2TP_ATTR_IP_DADDR]) {
+		cfg->local_ip.s_addr = nla_get_in_addr(attrs[L2TP_ATTR_IP_SADDR]);
+		cfg->peer_ip.s_addr = nla_get_in_addr(attrs[L2TP_ATTR_IP_DADDR]);
+		return 0;
+	}
+	return -EINVAL;
+}
+
 static int l2tp_nl_cmd_tunnel_create(struct sk_buff *skb, struct genl_info *info)
 {
 	u32 tunnel_id;
 	u32 peer_tunnel_id;
 	int proto_version;
-	int fd;
+	int fd = -1;
 	int ret = 0;
 	struct l2tp_tunnel_cfg cfg = { 0, };
 	struct l2tp_tunnel *tunnel;
@@ -191,33 +217,16 @@ static int l2tp_nl_cmd_tunnel_create(struct sk_buff *skb, struct genl_info *info
 	}
 	cfg.encap = nla_get_u16(attrs[L2TP_ATTR_ENCAP_TYPE]);
 
-	fd = -1;
+	/* Managed tunnels take the tunnel socket from userspace.
+	 * Unmanaged tunnels must call out the source and destination addresses
+	 * for the kernel to create the tunnel socket itself.
+	 */
 	if (attrs[L2TP_ATTR_FD]) {
 		fd = nla_get_u32(attrs[L2TP_ATTR_FD]);
 	} else {
-#if IS_ENABLED(CONFIG_IPV6)
-		if (attrs[L2TP_ATTR_IP6_SADDR] && attrs[L2TP_ATTR_IP6_DADDR]) {
-			cfg.local_ip6 = nla_data(attrs[L2TP_ATTR_IP6_SADDR]);
-			cfg.peer_ip6 = nla_data(attrs[L2TP_ATTR_IP6_DADDR]);
-		} else {
-#endif
-		if (attrs[L2TP_ATTR_IP_SADDR] && attrs[L2TP_ATTR_IP_DADDR]) {
-			cfg.local_ip.s_addr = nla_get_in_addr(attrs[L2TP_ATTR_IP_SADDR]);
-			cfg.peer_ip.s_addr = nla_get_in_addr(attrs[L2TP_ATTR_IP_DADDR]);
-		} else {
-			ret = -EINVAL;
+		ret = l2tp_nl_cmd_tunnel_create_get_addr(attrs, &cfg);
+		if (ret < 0)
 			goto out;
-		}
-		if (attrs[L2TP_ATTR_UDP_SPORT])
-			cfg.local_udp_port = nla_get_u16(attrs[L2TP_ATTR_UDP_SPORT]);
-		if (attrs[L2TP_ATTR_UDP_DPORT])
-			cfg.peer_udp_port = nla_get_u16(attrs[L2TP_ATTR_UDP_DPORT]);
-		cfg.use_udp_checksums = nla_get_flag(attrs[L2TP_ATTR_UDP_CSUM]);
-
-#if IS_ENABLED(CONFIG_IPV6)
-		cfg.udp6_zero_tx_checksums = nla_get_flag(attrs[L2TP_ATTR_UDP_ZERO_CSUM6_TX]);
-		cfg.udp6_zero_rx_checksums = nla_get_flag(attrs[L2TP_ATTR_UDP_ZERO_CSUM6_RX]);
-#endif
 	}
 
 	if (attrs[L2TP_ATTR_DEBUG])
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 22/29] l2tp: cleanup kzalloc calls
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (20 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 21/29] l2tp: cleanup netlink tunnel create address handling Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-21 17:32 ` [PATCH 23/29] l2tp: remove BUG_ON in l2tp_session_queue_purge Tom Parkin
                   ` (7 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

Passing "sizeof(struct blah)" in kzalloc calls is less readable,
potentially prone to future bugs if the type of the pointer is changed,
and triggers checkpatch warnings.

Tweak the kzalloc calls in l2tp which use this form to avoid the
warning.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 4973a0f035e3..b871cceeff7c 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1410,7 +1410,7 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
 	if (cfg)
 		encap = cfg->encap;
 
-	tunnel = kzalloc(sizeof(struct l2tp_tunnel), GFP_KERNEL);
+	tunnel = kzalloc(sizeof(*tunnel), GFP_KERNEL);
 	if (!tunnel) {
 		err = -ENOMEM;
 		goto err;
@@ -1647,7 +1647,7 @@ struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunn
 {
 	struct l2tp_session *session;
 
-	session = kzalloc(sizeof(struct l2tp_session) + priv_size, GFP_KERNEL);
+	session = kzalloc(sizeof(*session) + priv_size, GFP_KERNEL);
 	if (session) {
 		session->magic = L2TP_SESSION_MAGIC;
 		session->tunnel = tunnel;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 23/29] l2tp: remove BUG_ON in l2tp_session_queue_purge
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (21 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 22/29] l2tp: cleanup kzalloc calls Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-21 17:32 ` [PATCH 24/29] l2tp: remove BUG_ON in l2tp_tunnel_closeall Tom Parkin
                   ` (6 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

l2tp_session_queue_purge is only called from l2tp_core.c, and it's easy
to statically analyse the code paths calling it to validate that it
should never be passed a NULL session pointer.

Having a BUG_ON checking the session pointer triggers a checkpatch
warning.  Since the BUG_ON is of no value, remove it to avoid the
warning.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index b871cceeff7c..a1ed8baa5aaa 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -777,7 +777,6 @@ static int l2tp_session_queue_purge(struct l2tp_session *session)
 {
 	struct sk_buff *skb = NULL;
 
-	BUG_ON(!session);
 	BUG_ON(session->magic != L2TP_SESSION_MAGIC);
 	while ((skb = skb_dequeue(&session->reorder_q))) {
 		atomic_long_inc(&session->stats.rx_errors);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 24/29] l2tp: remove BUG_ON in l2tp_tunnel_closeall
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (22 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 23/29] l2tp: remove BUG_ON in l2tp_session_queue_purge Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-21 17:32 ` [PATCH 25/29] l2tp: don't BUG_ON session magic checks in l2tp_ppp Tom Parkin
                   ` (5 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

l2tp_tunnel_closeall is only called from l2tp_core.c, and it's easy
to statically analyse the code path calling it to validate that it
should never be passed a NULL tunnel pointer.

Having a BUG_ON checking the tunnel pointer triggers a checkpatch
warning.  Since the BUG_ON is of no value, remove it to avoid the
warning.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index a1ed8baa5aaa..6be3f2e69efd 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1188,8 +1188,6 @@ static void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel)
 	struct hlist_node *tmp;
 	struct l2tp_session *session;
 
-	BUG_ON(!tunnel);
-
 	l2tp_info(tunnel, L2TP_MSG_CONTROL, "%s: closing all sessions...\n",
 		  tunnel->name);
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 25/29] l2tp: don't BUG_ON session magic checks in l2tp_ppp
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (23 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 24/29] l2tp: remove BUG_ON in l2tp_tunnel_closeall Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-21 17:32 ` [PATCH 26/29] l2tp: don't BUG_ON seqfile " Tom Parkin
                   ` (4 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

checkpatch advises that WARN_ON and recovery code are preferred over
BUG_ON which crashes the kernel.

l2tp_ppp.c's BUG_ON checks of the l2tp session structure's "magic" field
occur in code paths where it's reasonably easy to recover:

 * In the case of pppol2tp_sock_to_session, we can return NULL and the
   caller will bail out appropriately.  There is no change required to
   any of the callsites of this function since they already handle
   pppol2tp_sock_to_session returning NULL.

 * In the case of pppol2tp_session_destruct we can just avoid
   decrementing the reference count on the suspect session structure.
   In the worst case scenario this results in a memory leak, which is
   preferable to a crash.

Convert these uses of BUG_ON to WARN_ON accordingly.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_ppp.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index e58fe7e3b884..6cd1a422c426 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -163,8 +163,12 @@ static inline struct l2tp_session *pppol2tp_sock_to_session(struct sock *sk)
 		sock_put(sk);
 		goto out;
 	}
-
-	BUG_ON(session->magic != L2TP_SESSION_MAGIC);
+	if (session->magic != L2TP_SESSION_MAGIC) {
+		WARN_ON(session->magic != L2TP_SESSION_MAGIC);
+		session = NULL;
+		sock_put(sk);
+		goto out;
+	}
 
 out:
 	return session;
@@ -419,8 +423,9 @@ static void pppol2tp_session_destruct(struct sock *sk)
 
 	if (session) {
 		sk->sk_user_data = NULL;
-		BUG_ON(session->magic != L2TP_SESSION_MAGIC);
-		l2tp_session_dec_refcount(session);
+		WARN_ON(session->magic != L2TP_SESSION_MAGIC);
+		if (session->magic == L2TP_SESSION_MAGIC)
+			l2tp_session_dec_refcount(session);
 	}
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 26/29] l2tp: don't BUG_ON seqfile checks in l2tp_ppp
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (24 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 25/29] l2tp: don't BUG_ON session magic checks in l2tp_ppp Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-21 17:32 ` [PATCH 27/29] l2tp: WARN_ON rather than BUG_ON in l2tp_session_queue_purge Tom Parkin
                   ` (3 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

checkpatch advises that WARN_ON and recovery code are preferred over
BUG_ON which crashes the kernel.

l2tp_ppp has a BUG_ON check of struct seq_file's private pointer in
pppol2tp_seq_start prior to accessing data through that pointer.

Rather than crashing, we can simply bail out early and return NULL in
order to terminate the seq file processing in much the same way as we do
when reaching the end of tunnel/session instances to render.

Retain a WARN_ON to help trace possible bugs in this area.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_ppp.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 6cd1a422c426..a58e0cc66e43 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -1479,7 +1479,12 @@ static void *pppol2tp_seq_start(struct seq_file *m, loff_t *offs)
 	if (!pos)
 		goto out;
 
-	BUG_ON(!m->private);
+	if (!m->private) {
+		WARN_ON(!m->private);
+		pd = NULL;
+		goto out;
+	}
+
 	pd = m->private;
 	net = seq_file_net(m);
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 27/29] l2tp: WARN_ON rather than BUG_ON in l2tp_session_queue_purge
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (25 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 26/29] l2tp: don't BUG_ON seqfile " Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-21 17:32 ` [PATCH 28/29] l2tp: remove BUG_ON refcount value in l2tp_session_free Tom Parkin
                   ` (2 subsequent siblings)
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

l2tp_session_queue_purge is used during session shutdown to drop any
skbs queued for reordering purposes according to L2TP dataplane rules.

The BUG_ON in this function checks the session magic feather in an
attempt to catch lifetime bugs.

Rather than crashing the kernel with a BUG_ON, we can simply WARN_ON and
refuse to do anything more -- in the worst case this could result in a
leak.  However this is highly unlikely given that the session purge only
occurs from codepaths which have obtained the session by means of a lookup
via. the parent tunnel and which check the session "dead" flag to
protect against shutdown races.

While we're here, have l2tp_session_queue_purge return void rather than
an integer, since neither of the callsites checked the return value.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 6be3f2e69efd..c7c513e0b042 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -773,16 +773,19 @@ EXPORT_SYMBOL(l2tp_recv_common);
 
 /* Drop skbs from the session's reorder_q
  */
-static int l2tp_session_queue_purge(struct l2tp_session *session)
+static void l2tp_session_queue_purge(struct l2tp_session *session)
 {
 	struct sk_buff *skb = NULL;
 
-	BUG_ON(session->magic != L2TP_SESSION_MAGIC);
+	if (session->magic != L2TP_SESSION_MAGIC) {
+		WARN_ON(session->magic != L2TP_SESSION_MAGIC);
+		return;
+	}
+
 	while ((skb = skb_dequeue(&session->reorder_q))) {
 		atomic_long_inc(&session->stats.rx_errors);
 		kfree_skb(skb);
 	}
-	return 0;
 }
 
 /* Internal UDP receive frame. Do the real work of receiving an L2TP data frame
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 28/29] l2tp: remove BUG_ON refcount value in l2tp_session_free
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (26 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 27/29] l2tp: WARN_ON rather than BUG_ON in l2tp_session_queue_purge Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-21 17:32 ` [PATCH 29/29] l2tp: WARN_ON rather than BUG_ON " Tom Parkin
  2020-07-21 23:19 ` [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings David Miller
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

l2tp_session_free is only called by l2tp_session_dec_refcount when the
reference count reaches zero, so it's of limited value to validate the
reference count value in l2tp_session_free itself.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index c7c513e0b042..34dacb14042f 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1565,8 +1565,6 @@ void l2tp_session_free(struct l2tp_session *session)
 {
 	struct l2tp_tunnel *tunnel = session->tunnel;
 
-	BUG_ON(refcount_read(&session->ref_count) != 0);
-
 	if (tunnel) {
 		BUG_ON(tunnel->magic != L2TP_TUNNEL_MAGIC);
 		l2tp_tunnel_dec_refcount(tunnel);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 29/29] l2tp: WARN_ON rather than BUG_ON in l2tp_session_free
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (27 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 28/29] l2tp: remove BUG_ON refcount value in l2tp_session_free Tom Parkin
@ 2020-07-21 17:32 ` Tom Parkin
  2020-07-21 23:19 ` [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings David Miller
  29 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-21 17:32 UTC (permalink / raw)
  To: netdev; +Cc: Tom Parkin

l2tp_session_free called BUG_ON if the tunnel magic feather value wasn't
correct.  The intent of this was to catch lifetime bugs; for example
early tunnel free due to incorrect use of reference counts.

Since the tunnel magic feather being wrong indicates either early free
or structure corruption, we can avoid doing more damage by simply
leaving the tunnel structure alone.  If the tunnel refcount isn't
dropped when it should be, the tunnel instance will remain in the
kernel, resulting in the tunnel structure and socket leaking.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 34dacb14042f..64c4350b8eb4 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1566,8 +1566,10 @@ void l2tp_session_free(struct l2tp_session *session)
 	struct l2tp_tunnel *tunnel = session->tunnel;
 
 	if (tunnel) {
-		BUG_ON(tunnel->magic != L2TP_TUNNEL_MAGIC);
-		l2tp_tunnel_dec_refcount(tunnel);
+		if (tunnel->magic == L2TP_TUNNEL_MAGIC)
+			l2tp_tunnel_dec_refcount(tunnel);
+		else
+			WARN_ON(tunnel->magic != L2TP_TUNNEL_MAGIC);
 	}
 
 	kfree(session);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [PATCH 05/29] l2tp: cleanup difficult-to-read line breaks
  2020-07-21 17:31 ` [PATCH 05/29] l2tp: cleanup difficult-to-read line breaks Tom Parkin
@ 2020-07-21 20:59   ` Jakub Kicinski
  2020-07-21 23:26     ` David Miller
  2020-07-22  9:06     ` Tom Parkin
  0 siblings, 2 replies; 38+ messages in thread
From: Jakub Kicinski @ 2020-07-21 20:59 UTC (permalink / raw)
  To: Tom Parkin; +Cc: netdev

On Tue, 21 Jul 2020 18:31:57 +0100 Tom Parkin wrote:
>  #if IS_ENABLED(CONFIG_IPV6)
> -		if (info->attrs[L2TP_ATTR_IP6_SADDR] &&
> -		    info->attrs[L2TP_ATTR_IP6_DADDR]) {
> -			cfg.local_ip6 = nla_data(
> -				info->attrs[L2TP_ATTR_IP6_SADDR]);
> -			cfg.peer_ip6 = nla_data(
> -				info->attrs[L2TP_ATTR_IP6_DADDR]);
> -		} else
> +		if (attrs[L2TP_ATTR_IP6_SADDR] && attrs[L2TP_ATTR_IP6_DADDR]) {
> +			cfg.local_ip6 = nla_data(attrs[L2TP_ATTR_IP6_SADDR]);
> +			cfg.peer_ip6 = nla_data(attrs[L2TP_ATTR_IP6_DADDR]);
> +		} else {
>  #endif

This no longer builds. Probably because you added the closing backet
which wasn't there?

Please make sure each patch in the submission builds cleanly.

Please split this submission into series of at most 15 patches at a
time, to make sure reviewers don't get overloaded.

Please CC people who are working on the l2tp code (get_maintainers
script is your friend).

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings
  2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
                   ` (28 preceding siblings ...)
  2020-07-21 17:32 ` [PATCH 29/29] l2tp: WARN_ON rather than BUG_ON " Tom Parkin
@ 2020-07-21 23:19 ` David Miller
  2020-07-22  9:07   ` Tom Parkin
  29 siblings, 1 reply; 38+ messages in thread
From: David Miller @ 2020-07-21 23:19 UTC (permalink / raw)
  To: tparkin; +Cc: netdev


This patch set is way too large to be reasonably reviewed by other
developers.

Please either find a way to combine some of the patches, or submit
this in stages of about 10 or so patches at a time.

I am not applying this submission as submitted.

Thank you.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 05/29] l2tp: cleanup difficult-to-read line breaks
  2020-07-21 20:59   ` Jakub Kicinski
@ 2020-07-21 23:26     ` David Miller
  2020-07-22  9:06     ` Tom Parkin
  1 sibling, 0 replies; 38+ messages in thread
From: David Miller @ 2020-07-21 23:26 UTC (permalink / raw)
  To: kuba; +Cc: tparkin, netdev

From: Jakub Kicinski <kuba@kernel.org>
Date: Tue, 21 Jul 2020 13:59:38 -0700

> Please split this submission into series of at most 15 patches at a
> time, to make sure reviewers don't get overloaded.

+1


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 05/29] l2tp: cleanup difficult-to-read line breaks
  2020-07-21 20:59   ` Jakub Kicinski
  2020-07-21 23:26     ` David Miller
@ 2020-07-22  9:06     ` Tom Parkin
  1 sibling, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-22  9:06 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: netdev

[-- Attachment #1: Type: text/plain, Size: 1212 bytes --]

On  Tue, Jul 21, 2020 at 13:59:38 -0700, Jakub Kicinski wrote:
> On Tue, 21 Jul 2020 18:31:57 +0100 Tom Parkin wrote:
> >  #if IS_ENABLED(CONFIG_IPV6)
> > -		if (info->attrs[L2TP_ATTR_IP6_SADDR] &&
> > -		    info->attrs[L2TP_ATTR_IP6_DADDR]) {
> > -			cfg.local_ip6 = nla_data(
> > -				info->attrs[L2TP_ATTR_IP6_SADDR]);
> > -			cfg.peer_ip6 = nla_data(
> > -				info->attrs[L2TP_ATTR_IP6_DADDR]);
> > -		} else
> > +		if (attrs[L2TP_ATTR_IP6_SADDR] && attrs[L2TP_ATTR_IP6_DADDR]) {
> > +			cfg.local_ip6 = nla_data(attrs[L2TP_ATTR_IP6_SADDR]);
> > +			cfg.peer_ip6 = nla_data(attrs[L2TP_ATTR_IP6_DADDR]);
> > +		} else {
> >  #endif
> 
> This no longer builds. Probably because you added the closing backet
> which wasn't there?
> 
> Please make sure each patch in the submission builds cleanly.

Sorry, this is a rebase snafu; my mistake.  I test-built the complete
patch series of course, but I'll test build each in turn for v2.

> Please split this submission into series of at most 15 patches at a
> time, to make sure reviewers don't get overloaded.

Will do.

> Please CC people who are working on the l2tp code (get_maintainers
> script is your friend).

Ack, thanks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings
  2020-07-21 23:19 ` [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings David Miller
@ 2020-07-22  9:07   ` Tom Parkin
  0 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-22  9:07 UTC (permalink / raw)
  To: David Miller; +Cc: netdev

[-- Attachment #1: Type: text/plain, Size: 530 bytes --]

On  Tue, Jul 21, 2020 at 16:19:17 -0700, David Miller wrote:
> 
> This patch set is way too large to be reasonably reviewed by other
> developers.
> 
> Please either find a way to combine some of the patches, or submit
> this in stages of about 10 or so patches at a time.
> 
> I am not applying this submission as submitted.
> 
> Thank you.

I will respin as requested, probably covering the more trivial
modifications in one series and the larger changes in a second
separate series.

Thanks for looking at it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 17/29] l2tp: avoid precidence issues in L2TP_SKB_CB macro
  2020-07-21 17:32 ` [PATCH 17/29] l2tp: avoid precidence issues in L2TP_SKB_CB macro Tom Parkin
@ 2020-07-28 16:21   ` Joe Perches
  2020-07-28 18:08     ` Joe Perches
  0 siblings, 1 reply; 38+ messages in thread
From: Joe Perches @ 2020-07-28 16:21 UTC (permalink / raw)
  To: Tom Parkin, netdev

On Tue, 2020-07-21 at 18:32 +0100, Tom Parkin wrote:
> checkpatch warned about the L2TP_SKB_CB macro's use of its argument: add
> braces to avoid the problem.
[]
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
[]
> @@ -93,7 +93,7 @@ struct l2tp_skb_cb {
>  	unsigned long		expires;
>  };
>  
> -#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *)&skb->cb[sizeof(struct inet_skb_parm)])
> +#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *)&(skb)->cb[sizeof(struct inet_skb_parm)])

Likely better to use a static inline.

Something like:

static inline struct l2tp_skb_cb *L2TP_SKB_SB(struct sk_buff *skb)
{
	return &skb->cb[sizeof(struct inet+skb_parm)];
}



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 17/29] l2tp: avoid precidence issues in L2TP_SKB_CB macro
  2020-07-28 16:21   ` Joe Perches
@ 2020-07-28 18:08     ` Joe Perches
  2020-07-28 19:31       ` Tom Parkin
  0 siblings, 1 reply; 38+ messages in thread
From: Joe Perches @ 2020-07-28 18:08 UTC (permalink / raw)
  To: Tom Parkin, netdev

On Tue, 2020-07-28 at 09:21 -0700, Joe Perches wrote:
> On Tue, 2020-07-21 at 18:32 +0100, Tom Parkin wrote:
> > checkpatch warned about the L2TP_SKB_CB macro's use of its argument: add
> > braces to avoid the problem.
> []
> > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> []
> > @@ -93,7 +93,7 @@ struct l2tp_skb_cb {
> >  	unsigned long		expires;
> >  };
> >  
> > -#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *)&skb->cb[sizeof(struct inet_skb_parm)])
> > +#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *)&(skb)->cb[sizeof(struct inet_skb_parm)])
> 
> Likely better to use a static inline.
> 
> Something like:
> 
> static inline struct l2tp_skb_cb *L2TP_SKB_SB(struct sk_buff *skb)
> {
> 	return &skb->cb[sizeof(struct inet+skb_parm)];
> }

More precisely:
---
 net/l2tp/l2tp_core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index e723828e458b..78ad6d8405c4 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -93,7 +93,10 @@ struct l2tp_skb_cb {
 	unsigned long		expires;
 };
 
-#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *)&(skb)->cb[sizeof(struct inet_skb_parm)])
+static inline struct l2tp_skb_cb *L2TP_SKB_CB(struct sk_buff *skb)
+{
+	return (struct l2tp_skb_cb *)&skb->cb[sizeof(struct inet_skb_parm)];
+}
 
 static struct workqueue_struct *l2tp_wq;
 



^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [PATCH 17/29] l2tp: avoid precidence issues in L2TP_SKB_CB macro
  2020-07-28 18:08     ` Joe Perches
@ 2020-07-28 19:31       ` Tom Parkin
  0 siblings, 0 replies; 38+ messages in thread
From: Tom Parkin @ 2020-07-28 19:31 UTC (permalink / raw)
  To: Joe Perches; +Cc: netdev

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

On  Tue, Jul 28, 2020 at 11:08:45 -0700, Joe Perches wrote:
> On Tue, 2020-07-28 at 09:21 -0700, Joe Perches wrote:
> > On Tue, 2020-07-21 at 18:32 +0100, Tom Parkin wrote:
> > > checkpatch warned about the L2TP_SKB_CB macro's use of its argument: add
> > > braces to avoid the problem.
> > []
> > > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> > []
> > > @@ -93,7 +93,7 @@ struct l2tp_skb_cb {
> > >  	unsigned long		expires;
> > >  };
> > >  
> > > -#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *)&skb->cb[sizeof(struct inet_skb_parm)])
> > > +#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *)&(skb)->cb[sizeof(struct inet_skb_parm)])
> > 
> > Likely better to use a static inline.
> > 
> > Something like:
> > 
> > static inline struct l2tp_skb_cb *L2TP_SKB_SB(struct sk_buff *skb)
> > {
> > 	return &skb->cb[sizeof(struct inet+skb_parm)];
> > }
> 
> More precisely:
> ---
>  net/l2tp/l2tp_core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index e723828e458b..78ad6d8405c4 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -93,7 +93,10 @@ struct l2tp_skb_cb {
>  	unsigned long		expires;
>  };
>  
> -#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *)&(skb)->cb[sizeof(struct inet_skb_parm)])
> +static inline struct l2tp_skb_cb *L2TP_SKB_CB(struct sk_buff *skb)
> +{
> +	return (struct l2tp_skb_cb *)&skb->cb[sizeof(struct inet_skb_parm)];
> +}
>  
>  static struct workqueue_struct *l2tp_wq;
>  
> 

Thanks Joe.  I can see this is better since we get some type checking
from the compiler for the function argument.

The patchset has been applied already, but I can try to integrate this
change in the future.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 38+ messages in thread

end of thread, other threads:[~2020-07-28 19:31 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-21 17:31 [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings Tom Parkin
2020-07-21 17:31 ` [PATCH 01/29] l2tp: cleanup whitespace around operators Tom Parkin
2020-07-21 17:31 ` [PATCH 02/29] l2tp: tweak comment style for consistency Tom Parkin
2020-07-21 17:31 ` [PATCH 03/29] l2tp: add a blank line following declarations Tom Parkin
2020-07-21 17:31 ` [PATCH 04/29] l2tp: cleanup excessive blank lines Tom Parkin
2020-07-21 17:31 ` [PATCH 05/29] l2tp: cleanup difficult-to-read line breaks Tom Parkin
2020-07-21 20:59   ` Jakub Kicinski
2020-07-21 23:26     ` David Miller
2020-07-22  9:06     ` Tom Parkin
2020-07-21 17:31 ` [PATCH 06/29] l2tp: cleanup wonky alignment of line-broken function calls Tom Parkin
2020-07-21 17:31 ` [PATCH 07/29] l2tp: cleanup suspect code indent Tom Parkin
2020-07-21 17:32 ` [PATCH 08/29] l2tp: add identifier name in function pointer prototype Tom Parkin
2020-07-21 17:32 ` [PATCH 09/29] l2tp: prefer using BIT macro Tom Parkin
2020-07-21 17:32 ` [PATCH 10/29] l2tp: cleanup comparisons to NULL Tom Parkin
2020-07-21 17:32 ` [PATCH 11/29] l2tp: cleanup unnecessary braces in if statements Tom Parkin
2020-07-21 17:32 ` [PATCH 12/29] l2tp: prefer seq_puts for unformatted output Tom Parkin
2020-07-21 17:32 ` [PATCH 13/29] l2tp: avoid multiple assignments Tom Parkin
2020-07-21 17:32 ` [PATCH 14/29] l2tp: line-break long function prototypes Tom Parkin
2020-07-21 17:32 ` [PATCH 15/29] l2tp: comment per net spinlock instances Tom Parkin
2020-07-21 17:32 ` [PATCH 16/29] l2tp: fix up incorrect comment in l2tp_recv_common Tom Parkin
2020-07-21 17:32 ` [PATCH 17/29] l2tp: avoid precidence issues in L2TP_SKB_CB macro Tom Parkin
2020-07-28 16:21   ` Joe Perches
2020-07-28 18:08     ` Joe Perches
2020-07-28 19:31       ` Tom Parkin
2020-07-21 17:32 ` [PATCH 18/29] l2tp: WARN_ON rather than BUG_ON in l2tp_debugfs.c Tom Parkin
2020-07-21 17:32 ` [PATCH 19/29] l2tp: use a function to render tunnel address in l2tp_debugfs Tom Parkin
2020-07-21 17:32 ` [PATCH 20/29] l2tp: cleanup netlink send of tunnel address information Tom Parkin
2020-07-21 17:32 ` [PATCH 21/29] l2tp: cleanup netlink tunnel create address handling Tom Parkin
2020-07-21 17:32 ` [PATCH 22/29] l2tp: cleanup kzalloc calls Tom Parkin
2020-07-21 17:32 ` [PATCH 23/29] l2tp: remove BUG_ON in l2tp_session_queue_purge Tom Parkin
2020-07-21 17:32 ` [PATCH 24/29] l2tp: remove BUG_ON in l2tp_tunnel_closeall Tom Parkin
2020-07-21 17:32 ` [PATCH 25/29] l2tp: don't BUG_ON session magic checks in l2tp_ppp Tom Parkin
2020-07-21 17:32 ` [PATCH 26/29] l2tp: don't BUG_ON seqfile " Tom Parkin
2020-07-21 17:32 ` [PATCH 27/29] l2tp: WARN_ON rather than BUG_ON in l2tp_session_queue_purge Tom Parkin
2020-07-21 17:32 ` [PATCH 28/29] l2tp: remove BUG_ON refcount value in l2tp_session_free Tom Parkin
2020-07-21 17:32 ` [PATCH 29/29] l2tp: WARN_ON rather than BUG_ON " Tom Parkin
2020-07-21 23:19 ` [PATCH net-next 00/29] l2tp: cleanup checkpatch.pl warnings David Miller
2020-07-22  9:07   ` Tom Parkin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.