[PATCH v2 0/5] Fix net/core W=1 warnings

[PATCH v2 0/5] Fix net/core W=1 warnings

[Bart Van Assche]

Hi Dave,

This patch series addresses 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.

Changes compared to v1:
- Dropped two patches from this series.
- Rebased this patch series on top of
  git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git

Bart Van Assche (5):
  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/datagram.c       |  2 ++
 net/core/datagram.h       | 15 +++++++++++++++
 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 ++
 7 files changed, 31 insertions(+), 7 deletions(-)
 create mode 100644 net/core/datagram.h

-- 
2.21.0.155.ge902e9bcae20

[PATCH v2 1/5] 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 v2 2/5] 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 v2 3/5] 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 v2 4/5] 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 v2 5/5] 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 ed8accb17418..0dafec5cada0 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

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

[Sabrina Dubroca]

2019-03-25, 09:17:23 -0700, Bart Van Assche wrote:
> 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_ */

That's rather ugly. Could it just be moved to an appropriate file in
include/?

-- 
Sabrina

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

[Bart Van Assche]

On Mon, 2019-03-25 at 19:26 +0100, Sabrina Dubroca wrote:
> 2019-03-25, 09:17:23 -0700, Bart Van Assche wrote:
> > 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_ */
> 
> That's rather ugly. Could it just be moved to an appropriate file in
> include/?

Hi Sabrina,

I think the convention in the Linux kernel is to keep header files with local
declarations in the source code directory and only to declare functions that
are used by other kernel components under include/. Do you think that the
function __zerocopy_sg_from_iter() will be needed by other kernel components?

Thanks,

Bart.

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

[Sabrina Dubroca]

2019-03-26, 10:11:34 -0700, Bart Van Assche wrote:
> On Mon, 2019-03-25 at 19:26 +0100, Sabrina Dubroca wrote:
> > 2019-03-25, 09:17:23 -0700, Bart Van Assche wrote:
> > > 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_ */
> > 
> > That's rather ugly. Could it just be moved to an appropriate file in
> > include/?
> 
> Hi Sabrina,
> 
> I think the convention in the Linux kernel is to keep header files with local
> declarations in the source code directory and only to declare functions that
> are used by other kernel components under include/.

I didn't realize it was a common practice.

> Do you think that the function __zerocopy_sg_from_iter() will be
> needed by other kernel components?

No idea. It has a couple of wrappers, so maybe not.

The patch looks ok, sorry for the noise.

-- 
Sabrina

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

[Al Viro]

On Mon, Mar 25, 2019 at 07:26:42PM +0100, Sabrina Dubroca wrote:
> 2019-03-25, 09:17:23 -0700, Bart Van Assche wrote:
> > 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_ */
> 
> That's rather ugly. Could it just be moved to an appropriate file in
> include/?

Dumping everything into widely-included files is a Bloody Bad Idea(tm);
it makes reasoning about the code much harder.

If anything, we should trim the hell out of those; details that matter
only to a well-defined subset of the kernel should be local to it.
Consider, for example, include/net/af_unix.h.  The stuff defined in
there:

Externs for functions:
unix_inflight, unix_notinflight, unix_destruct_scm, unix_gc, wait_for_unix_gc,
unix_get_socket, unix_peer_get, unix_sysctl_register, unix_sysctl_unregister,
unix_inq_len, unix_outq_len

Constants: UNIX_HASH_SIZE, UNIX_HASH_BITS

Variables: unix_tot_inflight, unix_table_lock, unix_socket_table.

Types: struct unix_address, struct unix_skb_parms, struct unix_sock.

Function-like macros and inlines: UNIXCB, unix_state_lock, unix_state_unlock,
unix_state_lock_nested, unix_sk

Convenience macro (and quite likely a namespace pollution, at that): peer_wait

Now, uses outside of net/unix/*.c and include/net/af_unix.h itself:

fs/io_uring.c:2063:     unix_destruct_scm(skb);
fs/io_uring.c:2101:             unix_inflight(fpl->user, fpl->fp[i]);
fs/io_uring.c:2105:     UNIXCB(skb).fp = fpl;
security/lsm_audit.c:323:                       struct unix_sock *u;
security/lsm_audit.c:324:                       struct unix_address *addr;
security/lsm_audit.c:354:                               u = unix_sk(sk);

and that's it.  Which is nice to realize, especially since it means that
you don't need to be concerned about something unexpected poking in
unix_socket_table internals, etc.

And actually looking at these two places outside of net/unix, you'll
see that
	* lsm_audit.c one is Linux S&M poking its fingers where they
do not belong (as usual) and, until the last cycle, screwing it up (ditto).
	* io_uring.c is a new addition, open-coding what probably
ought to be a few better-defined primitives - it's accessing the
af_unix.c guts on too low level.

Most of the af_unix.h contents definitely has no business being
visible anywhere in include/* - it should be in net/unix/ instead;
the remaining bits ought to be compactified into a sane and narrow API.
Figuring that API out is a non-trivial work, but it needs to be done.

Dumping internal details into include/* makes life much harder
when working with the code, trying to understand it, etc.
The usual reasons to separate interfaces and internals do
apply in the kernel.

Note, BTW, that stale junk (extern for a function removed at some
point, etc.) tends to stay around, confusing the hell out of readers.
And include/* tends to be considerably more sticky in that respect.

The bottom line: keep public headers tidy; internal details belong
with the code working with those.

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

[Sabrina Dubroca]

2019-03-26, 18:17:58 +0000, Al Viro wrote:
> Dumping everything into widely-included files is a Bloody Bad Idea(tm);
> it makes reasoning about the code much harder.
> 
> If anything, we should trim the hell out of those; details that matter
> only to a well-defined subset of the kernel should be local to it.
> Consider, for example, include/net/af_unix.h.  The stuff defined in
> there:
> 
[snip example]
> 
> Dumping internal details into include/* makes life much harder
> when working with the code, trying to understand it, etc.
> The usual reasons to separate interfaces and internals do
> apply in the kernel.
> 
> Note, BTW, that stale junk (extern for a function removed at some
> point, etc.) tends to stay around, confusing the hell out of readers.
> And include/* tends to be considerably more sticky in that respect.
> 
> The bottom line: keep public headers tidy; internal details belong
> with the code working with those.

Uh, yeah, makes sense. Thanks for the detailed example, I didn't think
the include/* situation was that bad.

-- 
Sabrina