[PATCH 0/7] net/core patches for kernel v5.2

[PATCH 0/7] net/core patches for kernel v5.2

[Bart Van Assche]

Hi Dave,

While inspecting the code of inet6_pton() I found an open-coded strlcpy()
implementation in that function. The last patch in this series simplifies
inet6_pton() and the first six patches address the compiler warnings reported
when building the code in net/core with W=1. Please consider this patch
series for kernel v5.2.

Thanks,

Bart.

Bart Van Assche (7):
  net/core: Document reuseport_add_sock() bind_inany argument
  net/core: Document all dev_ioctl() arguments
  net/core: Document __skb_flow_dissect() flags argument
  net/core: Fix rtnetlink kernel-doc headers
  net/core: Allow the compiler to verify declaration and definition
    consistency
  net/core: Declare a local function 'static'
  net/utils: Use strlcpy() instead of open-coding it

 net/core/datagram.c       |  3 +++
 net/core/datagram.h       | 13 +++++++++++++
 net/core/dev_ioctl.c      |  3 ++-
 net/core/flow_dissector.c |  2 ++
 net/core/rtnetlink.c      |  9 ++++++---
 net/core/skbuff.c         |  5 ++---
 net/core/sock_reuseport.c |  2 ++
 net/core/utils.c          |  6 +-----
 8 files changed, 31 insertions(+), 12 deletions(-)
 create mode 100644 net/core/datagram.h

-- 
2.21.0.155.ge902e9bcae20

[PATCH 1/7] net/core: Document reuseport_add_sock() bind_inany argument

[Bart Van Assche]

This patch avoids that the following warning is reported when building
with W=1:

warning: Function parameter or member 'bind_inany' not described in 'reuseport_add_sock'

Cc: Martin KaFai Lau <kafai@fb.com>
Fixes: 2dbb9b9e6df6 ("bpf: Introduce BPF_PROG_TYPE_SK_REUSEPORT") # v4.19.
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 net/core/sock_reuseport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/sock_reuseport.c b/net/core/sock_reuseport.c
index d8fe3e549373..dc4aefdf2a08 100644
--- a/net/core/sock_reuseport.c
+++ b/net/core/sock_reuseport.c
@@ -144,6 +144,8 @@ static void reuseport_free_rcu(struct rcu_head *head)
  *  reuseport_add_sock - Add a socket to the reuseport group of another.
  *  @sk:  New socket to add to the group.
  *  @sk2: Socket belonging to the existing reuseport group.
+ *  @bind_inany: Whether or not the group is bound to a local INANY address.
+ *
  *  May return ENOMEM and not add socket to group under memory pressure.
  */
 int reuseport_add_sock(struct sock *sk, struct sock *sk2, bool bind_inany)
-- 
2.21.0.155.ge902e9bcae20

[PATCH 2/7] net/core: Document all dev_ioctl() arguments

[Bart Van Assche]

This patch avoids that the following warnings are reported when building
with W=1:

net/core/dev_ioctl.c:378: warning: Function parameter or member 'ifr' not described in 'dev_ioctl'
net/core/dev_ioctl.c:378: warning: Function parameter or member 'need_copyout' not described in 'dev_ioctl'
net/core/dev_ioctl.c:378: warning: Excess function parameter 'arg' description in 'dev_ioctl'

Cc: Al Viro <viro@zeniv.linux.org.uk>
Fixes: 44c02a2c3dc5 ("dev_ioctl(): move copyin/copyout to callers") # v4.16.
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 net/core/dev_ioctl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 31380fd5a4e2..5163d900bb4f 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -366,7 +366,8 @@ EXPORT_SYMBOL(dev_load);
  *	dev_ioctl	-	network device ioctl
  *	@net: the applicable net namespace
  *	@cmd: command to issue
- *	@arg: pointer to a struct ifreq in user space
+ *	@ifr: pointer to a struct ifreq in user space
+ *	@need_copyout: whether or not copy_to_user() should be called
  *
  *	Issue ioctl functions to devices. This is normally called by the
  *	user space syscall interfaces but can sometimes be useful for
-- 
2.21.0.155.ge902e9bcae20

[PATCH 3/7] net/core: Document __skb_flow_dissect() flags argument

[Bart Van Assche]

This patch avoids that the following warning is reported when building
with W=1:

warning: Function parameter or member 'flags' not described in '__skb_flow_dissect'

Cc: Tom Herbert <tom@herbertland.com>
Fixes: cd79a2382aa5 ("flow_dissector: Add flags argument to skb_flow_dissector functions") # v4.3.
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 net/core/flow_dissector.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index bb1a54747d64..b4d581134ef2 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -732,6 +732,8 @@ bool __skb_flow_bpf_dissect(struct bpf_prog *prog,
  * @proto: protocol for which to get the flow, if @data is NULL use skb->protocol
  * @nhoff: network header offset, if @data is NULL use skb_network_offset(skb)
  * @hlen: packet header length, if @data is NULL use skb_headlen(skb)
+ * @flags: flags that control the dissection process, e.g.
+ *         FLOW_DISSECTOR_F_STOP_AT_L3.
  *
  * The function will try to retrieve individual keys into target specified
  * by flow_dissector from either the skbuff or a raw buffer specified by the
-- 
2.21.0.155.ge902e9bcae20

[PATCH 4/7] net/core: Fix rtnetlink kernel-doc headers

[Bart Van Assche]

This patch avoids that the following warnings are reported when building
with W=1:

net/core/rtnetlink.c:3580: warning: Function parameter or member 'ndm' not described in 'ndo_dflt_fdb_add'
net/core/rtnetlink.c:3580: warning: Function parameter or member 'tb' not described in 'ndo_dflt_fdb_add'
net/core/rtnetlink.c:3580: warning: Function parameter or member 'dev' not described in 'ndo_dflt_fdb_add'
net/core/rtnetlink.c:3580: warning: Function parameter or member 'addr' not described in 'ndo_dflt_fdb_add'
net/core/rtnetlink.c:3580: warning: Function parameter or member 'vid' not described in 'ndo_dflt_fdb_add'
net/core/rtnetlink.c:3580: warning: Function parameter or member 'flags' not described in 'ndo_dflt_fdb_add'
net/core/rtnetlink.c:3718: warning: Function parameter or member 'ndm' not described in 'ndo_dflt_fdb_del'
net/core/rtnetlink.c:3718: warning: Function parameter or member 'tb' not described in 'ndo_dflt_fdb_del'
net/core/rtnetlink.c:3718: warning: Function parameter or member 'dev' not described in 'ndo_dflt_fdb_del'
net/core/rtnetlink.c:3718: warning: Function parameter or member 'addr' not described in 'ndo_dflt_fdb_del'
net/core/rtnetlink.c:3718: warning: Function parameter or member 'vid' not described in 'ndo_dflt_fdb_del'
net/core/rtnetlink.c:3861: warning: Function parameter or member 'skb' not described in 'ndo_dflt_fdb_dump'
net/core/rtnetlink.c:3861: warning: Function parameter or member 'cb' not described in 'ndo_dflt_fdb_dump'
net/core/rtnetlink.c:3861: warning: Function parameter or member 'filter_dev' not described in 'ndo_dflt_fdb_dump'
net/core/rtnetlink.c:3861: warning: Function parameter or member 'idx' not described in 'ndo_dflt_fdb_dump'
net/core/rtnetlink.c:3861: warning: Excess function parameter 'nlh' description in 'ndo_dflt_fdb_dump'

Cc: Hubert Sokolowski <hubert.sokolowski@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 net/core/rtnetlink.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a51cab95ba64..f9b964fd4e4d 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3569,7 +3569,7 @@ static void rtnl_fdb_notify(struct net_device *dev, u8 *addr, u16 vid, int type,
 	rtnl_set_sk_err(net, RTNLGRP_NEIGH, err);
 }
 
-/**
+/*
  * ndo_dflt_fdb_add - default netdevice operation to add an FDB entry
  */
 int ndo_dflt_fdb_add(struct ndmsg *ndm,
@@ -3708,7 +3708,7 @@ static int rtnl_fdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 	return err;
 }
 
-/**
+/*
  * ndo_dflt_fdb_del - default netdevice operation to delete an FDB entry
  */
 int ndo_dflt_fdb_del(struct ndmsg *ndm,
@@ -3847,8 +3847,11 @@ static int nlmsg_populate_fdb(struct sk_buff *skb,
 
 /**
  * ndo_dflt_fdb_dump - default netdevice operation to dump an FDB table.
- * @nlh: netlink message header
+ * @skb: socket buffer to store message in
+ * @cb: netlink callback
  * @dev: netdevice
+ * @filter_dev: ignored
+ * @idx: the number of FDB table entries dumped is added to *@idx
  *
  * Default netdevice operation to dump the existing unicast address list.
  * Returns number of addresses from list put in skb.
-- 
2.21.0.155.ge902e9bcae20

[PATCH 5/7] net/core: Allow the compiler to verify declaration and definition consistency

[Bart Van Assche]

Instead of declaring a function in a .c file, declare it in a header
file and include that header file from the source files that define
and that use the function. That allows the compiler to verify
consistency of declaration and definition. See also commit
52267790ef52 ("sock: add MSG_ZEROCOPY") # v4.14.

Cc: Willem de Bruijn <willemb@google.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 net/core/datagram.c |  2 ++
 net/core/datagram.h | 15 +++++++++++++++
 net/core/skbuff.c   |  5 ++---
 3 files changed, 19 insertions(+), 3 deletions(-)
 create mode 100644 net/core/datagram.h

diff --git a/net/core/datagram.c b/net/core/datagram.c
index b2651bb6d2a3..c42f1f0e62b6 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c
@@ -61,6 +61,8 @@
 #include <trace/events/skb.h>
 #include <net/busy_poll.h>
 
+#include "datagram.h"
+
 /*
  *	Is a socket 'connection oriented' ?
  */
diff --git a/net/core/datagram.h b/net/core/datagram.h
new file mode 100644
index 000000000000..bcfb75bfa3b2
--- /dev/null
+++ b/net/core/datagram.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _NET_CORE_DATAGRAM_H_
+#define _NET_CORE_DATAGRAM_H_
+
+#include <linux/types.h>
+
+struct sock;
+struct sk_buff;
+struct iov_iter;
+
+int __zerocopy_sg_from_iter(struct sock *sk, struct sk_buff *skb,
+			    struct iov_iter *from, size_t length);
+
+#endif /* _NET_CORE_DATAGRAM_H_ */
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 2415d9cb9b89..4782f9354dd1 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -77,6 +77,8 @@
 #include <linux/capability.h>
 #include <linux/user_namespace.h>
 
+#include "datagram.h"
+
 struct kmem_cache *skbuff_head_cache __ro_after_init;
 static struct kmem_cache *skbuff_fclone_cache __ro_after_init;
 #ifdef CONFIG_SKB_EXTENSIONS
@@ -1105,9 +1107,6 @@ void sock_zerocopy_put_abort(struct ubuf_info *uarg, bool have_uref)
 }
 EXPORT_SYMBOL_GPL(sock_zerocopy_put_abort);
 
-extern int __zerocopy_sg_from_iter(struct sock *sk, struct sk_buff *skb,
-				   struct iov_iter *from, size_t length);
-
 int skb_zerocopy_iter_dgram(struct sk_buff *skb, struct msghdr *msg, int len)
 {
 	return __zerocopy_sg_from_iter(skb->sk, skb, &msg->msg_iter, len);
-- 
2.21.0.155.ge902e9bcae20

[PATCH 6/7] net/core: Declare a local function 'static'

[Bart Van Assche]

This patch avoids that the compiler complains about a missing prototype
when building with W=1.

Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 net/core/datagram.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/core/datagram.c b/net/core/datagram.c
index c42f1f0e62b6..334e0a7ce940 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c
@@ -410,6 +410,7 @@ int skb_kill_datagram(struct sock *sk, struct sk_buff *skb, unsigned int flags)
 }
 EXPORT_SYMBOL(skb_kill_datagram);
 
+static
 int __skb_datagram_iter(const struct sk_buff *skb, int offset,
 			struct iov_iter *to, int len, bool fault_short,
 			size_t (*cb)(const void *, size_t, void *, struct iov_iter *),
-- 
2.21.0.155.ge902e9bcae20

[PATCH 7/7] net/utils: Use strlcpy() instead of open-coding it

[Bart Van Assche]

This patch does not change any functionality but makes the code easier
to read.

Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 net/core/utils.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/net/core/utils.c b/net/core/utils.c
index 2a597ac7808e..5f11e89f173f 100644
--- a/net/core/utils.c
+++ b/net/core/utils.c
@@ -339,12 +339,8 @@ static int inet6_pton(struct net *net, const char *src, u16 port_num,
 	    src + srclen != scope_delim && *scope_delim == '%') {
 		struct net_device *dev;
 		char scope_id[16];
-		size_t scope_len = min_t(size_t, sizeof(scope_id) - 1,
-					 src + srclen - scope_delim - 1);
-
-		memcpy(scope_id, scope_delim + 1, scope_len);
-		scope_id[scope_len] = '\0';
 
+		strlcpy(scope_id, scope_delim + 1, sizeof(scope_id));
 		dev = dev_get_by_name(net, scope_id);
 		if (dev) {
 			addr6->sin6_scope_id = dev->ifindex;
-- 
2.21.0.155.ge902e9bcae20

Re: [PATCH 7/7] net/utils: Use strlcpy() instead of open-coding it

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 6/7] net/core: Declare a local function 'static'

[Sagi Grimberg]

This can be dropped. Dave merged a fix for it already

On 3/21/19 3:19 PM, Bart Van Assche wrote:
> This patch avoids that the compiler complains about a missing prototype
> when building with W=1.
> 
> Cc: Sagi Grimberg <sagi@grimberg.me>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   net/core/datagram.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/net/core/datagram.c b/net/core/datagram.c
> index c42f1f0e62b6..334e0a7ce940 100644
> --- a/net/core/datagram.c
> +++ b/net/core/datagram.c
> @@ -410,6 +410,7 @@ int skb_kill_datagram(struct sock *sk, struct sk_buff *skb, unsigned int flags)
>   }
>   EXPORT_SYMBOL(skb_kill_datagram);
>   
> +static
>   int __skb_datagram_iter(const struct sk_buff *skb, int offset,
>   			struct iov_iter *to, int len, bool fault_short,
>   			size_t (*cb)(const void *, size_t, void *, struct iov_iter *),
> 

Re: [PATCH 7/7] net/utils: Use strlcpy() instead of open-coding it

[Rustad, Mark D]

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

On Mar 21, 2019, at 3:19 PM, Bart Van Assche <bvanassche@acm.org> wrote:

> This patch does not change any functionality but makes the code easier
> to read.
>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  net/core/utils.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/net/core/utils.c b/net/core/utils.c
> index 2a597ac7808e..5f11e89f173f 100644
> --- a/net/core/utils.c
> +++ b/net/core/utils.c
> @@ -339,12 +339,8 @@ static int inet6_pton(struct net *net, const char  
> *src, u16 port_num,
>  	    src + srclen != scope_delim && *scope_delim == '%') {
>  		struct net_device *dev;
>  		char scope_id[16];
> -		size_t scope_len = min_t(size_t, sizeof(scope_id) - 1,
> -					 src + srclen - scope_delim - 1);
> -
> -		memcpy(scope_id, scope_delim + 1, scope_len);
> -		scope_id[scope_len] = '\0';
>
> +		strlcpy(scope_id, scope_delim + 1, sizeof(scope_id));
>  		dev = dev_get_by_name(net, scope_id);
>  		if (dev) {
>  			addr6->sin6_scope_id = dev->ifindex;
> --
> 2.21.0.155.ge902e9bcae20


This doesn't look right to me. The original code seemingly would stop the  
copy before getting to a possible '%' character. The new code only stops at  
eol. So this appears to change the functionality and I assume is broken in  
any case that uses the %. Take another look at what that min_t is really  
doing.

--
Mark Rustad, Networking Division, Intel Corporation

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH 0/7] net/core patches for kernel v5.2

[David Miller]

From: Bart Van Assche <bvanassche@acm.org>
Date: Thu, 21 Mar 2019 15:19:39 -0700

> While inspecting the code of inet6_pton() I found an open-coded strlcpy()
> implementation in that function. The last patch in this series simplifies
> inet6_pton() and the first six patches address the compiler warnings reported
> when building the code in net/core with W=1. Please consider this patch
> series for kernel v5.2.

Please drop patch #6 and respond to the feedback for patch #7.

Thanks.

Re: [PATCH 7/7] net/utils: Use strlcpy() instead of open-coding it

[Bart Van Assche]

On Fri, 2019-03-22 at 18:46 +0000, Rustad, Mark D wrote:
> This doesn't look right to me. The original code seemingly would stop the  
> copy before getting to a possible '%' character. The new code only stops at  
> eol. So this appears to change the functionality and I assume is broken in  
> any case that uses the %. Take another look at what that min_t is really  
> doing.

Hi Mark,

Thanks for the feedback. I will drop this patch.

Bart.