netdev - Re: [PATCH 5/6] net/core: support compiling out splice

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20141122214853.GB23711@thin>
Date:	Sat, 22 Nov 2014 13:48:53 -0800
From:	Josh Triplett <josh@...htriplett.org>
To:	Pieter Smith <pieter@...sman.nl>
Cc:	"David S. Miller" <davem@...emloft.net>,
	Tom Herbert <therbert@...gle.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Daniel Borkmann <dborkman@...hat.com>,
	Willem de Bruijn <willemb@...gle.com>,
	"Michael S. Tsirkin" <mst@...hat.com>,
	Alexander Duyck <alexander.h.duyck@...el.com>,
	Paul Durrant <Paul.Durrant@...rix.com>,
	Thomas Graf <tgraf@...g.ch>, Jan Beulich <JBeulich@...e.com>,
	Miklos Szeredi <mszeredi@...e.cz>,
	open list <linux-kernel@...r.kernel.org>,
	"open list:NETWORKING [GENERAL]" <netdev@...r.kernel.org>
Subject: Re: [PATCH 5/6] net/core: support compiling out splice

On Sat, Nov 22, 2014 at 10:00:00PM +0100, Pieter Smith wrote:
> Compile out splice support from networking core when the splice-family of
> syscalls is not supported by the system (i.e. CONFIG_SYSCALL_SPLICE is
> undefined).

Please explain in the commit message why this particular bit of splice
support needs compiling out when most other bits do not.

Also, a couple of style comments below.

> Signed-off-by: Pieter Smith <pieter@...sman.nl>
> ---
>  include/linux/skbuff.h | 9 +++++++++
>  net/core/skbuff.c      | 9 ++++++---
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index a59d934..8c28524 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -2640,9 +2640,18 @@ int skb_copy_bits(const struct sk_buff *skb, int offset, void *to, int len);
>  int skb_store_bits(struct sk_buff *skb, int offset, const void *from, int len);
>  __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset, u8 *to,
>  			      int len, __wsum csum);
> +#ifdef CONFIG_SYSCALL_SPLICE
>  int skb_splice_bits(struct sk_buff *skb, unsigned int offset,
>  		    struct pipe_inode_info *pipe, unsigned int len,
>  		    unsigned int flags);
> +#else /* #ifdef CONFIG_SYSCALL_SPLICE */
> +static inline int skb_splice_bits(struct sk_buff *skb, unsigned int offset,
> +		    struct pipe_inode_info *pipe, unsigned int len,
> +		    unsigned int flags)
> +{
> +	return -EPERM;
> +}
> +#endif /* #ifdef CONFIG_SYSCALL_SPLICE */

These comments, and the one added below in the corresponding .c file,
don't match the prevailing style.  These are so short (fitting on one
screen) that they hardly seem worth the comments at all, but if you want
to include them, s/#ifdef// in the comment.

>  void skb_copy_and_csum_dev(const struct sk_buff *skb, u8 *to);
>  unsigned int skb_zerocopy_headlen(const struct sk_buff *from);
>  int skb_zerocopy(struct sk_buff *to, struct sk_buff *from,
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 61059a0..74fad8a 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1781,9 +1781,9 @@ static bool __splice_segment(struct page *page, unsigned int poff,
>   * Map linear and fragment data from the skb to spd. It reports true if the
>   * pipe is full or if we already spliced the requested length.
>   */
> -static bool __skb_splice_bits(struct sk_buff *skb, struct pipe_inode_info *pipe,
> -			      unsigned int *offset, unsigned int *len,
> -			      struct splice_pipe_desc *spd, struct sock *sk)
> +static bool __maybe_unused __skb_splice_bits(struct sk_buff *skb, struct pipe_inode_info *pipe,
> +					     unsigned int *offset, unsigned int *len,
> +					     struct splice_pipe_desc *spd, struct sock *sk)
>  {
>  	int seg;
>  
> @@ -1821,6 +1821,7 @@ static bool __skb_splice_bits(struct sk_buff *skb, struct pipe_inode_info *pipe,
>   * the frag list, if such a thing exists. We'd probably need to recurse to
>   * handle that cleanly.
>   */
> +#ifdef CONFIG_SYSCALL_SPLICE
>  int skb_splice_bits(struct sk_buff *skb, unsigned int offset,
>  		    struct pipe_inode_info *pipe, unsigned int tlen,
>  		    unsigned int flags)
> @@ -1876,6 +1877,8 @@ done:
>  
>  	return ret;
>  }
> +#endif /* #ifdef CONFIG_SYSCALL_SPLICE */
> +

This extra blank line shouldn't be here.

>  /**
>   *	skb_store_bits - store bits from kernel buffer to skb
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html