netdev - Re: [PATCH net-next v4 08/15] net/smc: Add ability to work with extended SMC netlink API

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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 11 Nov 2020 14:25:51 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Karsten Graul <kgraul@...ux.ibm.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        linux-s390@...r.kernel.org, hca@...ux.ibm.com, raspl@...ux.ibm.com
Subject: Re: [PATCH net-next v4 08/15] net/smc: Add ability to work with
 extended SMC netlink API

On Mon,  9 Nov 2020 16:18:07 +0100 Karsten Graul wrote:
> From: Guvenc Gulce <guvenc@...ux.ibm.com>
> 
> smc_diag module should be able to work with legacy and
> extended netlink api. This is done by using the sequence field
> of the netlink message header. Sequence field is optional and was
> filled with a constant value MAGIC_SEQ in the current
> implementation.
> New constant values MAGIC_SEQ_V2 and MAGIC_SEQ_V2_ACK are used to
> signal the usage of the new Netlink API between userspace and
> kernel.
> 
> Signed-off-by: Guvenc Gulce <guvenc@...ux.ibm.com>
> Signed-off-by: Karsten Graul <kgraul@...ux.ibm.com>

> diff --git a/include/uapi/linux/smc_diag.h b/include/uapi/linux/smc_diag.h
> index 8cb3a6fef553..236c1c52d562 100644
> --- a/include/uapi/linux/smc_diag.h
> +++ b/include/uapi/linux/smc_diag.h
> @@ -6,6 +6,13 @@
>  #include <linux/inet_diag.h>
>  #include <rdma/ib_user_verbs.h>
>  
> +/* Sequence numbers */
> +enum {
> +	MAGIC_SEQ = 123456,
> +	MAGIC_SEQ_V2,
> +	MAGIC_SEQ_V2_ACK,
> +};
> +
>  /* Request structure */
>  struct smc_diag_req {
>  	__u8	diag_family;
> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
> index 44be723c97fe..bc2b616524ff 100644
> --- a/net/smc/smc_diag.c
> +++ b/net/smc/smc_diag.c
> @@ -293,19 +293,24 @@ static int smc_diag_dump(struct sk_buff *skb, struct netlink_callback *cb)
>  	return skb->len;
>  }
>  
> +static int smc_diag_dump_ext(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> +	return skb->len;
> +}
> +
>  static int smc_diag_handler_dump(struct sk_buff *skb, struct nlmsghdr *h)
>  {
>  	struct net *net = sock_net(skb->sk);
> -

Why did you drop the new line separating variables from code?

> +	struct netlink_dump_control c = {
> +		.min_dump_alloc = SKB_WITH_OVERHEAD(32768),
> +	};
>  	if (h->nlmsg_type == SOCK_DIAG_BY_FAMILY &&
>  	    h->nlmsg_flags & NLM_F_DUMP) {
> -		{
> -			struct netlink_dump_control c = {
> -				.dump = smc_diag_dump,
> -				.min_dump_alloc = SKB_WITH_OVERHEAD(32768),
> -			};
> -			return netlink_dump_start(net->diag_nlsk, skb, h, &c);
> -		}
> +		if (h->nlmsg_seq >= MAGIC_SEQ_V2)

This is not checked by the kernel, how do you know all user space
currently passes 123456?

Also, obviously, this is a rather weird abuse of sequence numbers.

Why don't you just add new attributes for new stuff you want to expose?
That's never mentioned anywhere, AFAICS.

> +			c.dump = smc_diag_dump_ext;
> +		else
> +			c.dump = smc_diag_dump;
> +		return netlink_dump_start(net->diag_nlsk, skb, h, &c);
>  	}
>  	return 0;
>  }