diff mbox series

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

Message ID 20201109151814.15040-9-kgraul@linux.ibm.com (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series net/smc: extend diagnostic netlink interface | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 45 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Karsten Graul Nov. 9, 2020, 3:18 p.m. UTC
From: Guvenc Gulce <guvenc@linux.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@linux.ibm.com>
Signed-off-by: Karsten Graul <kgraul@linux.ibm.com>
---
 include/uapi/linux/smc_diag.h |  7 +++++++
 net/smc/smc_diag.c            | 21 +++++++++++++--------
 2 files changed, 20 insertions(+), 8 deletions(-)

Comments

Jakub Kicinski Nov. 11, 2020, 10:25 p.m. UTC | #1
On Mon,  9 Nov 2020 16:18:07 +0100 Karsten Graul wrote:
> From: Guvenc Gulce <guvenc@linux.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@linux.ibm.com>
> Signed-off-by: Karsten Graul <kgraul@linux.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;
>  }
diff mbox series

Patch

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);
-
+	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)
+			c.dump = smc_diag_dump_ext;
+		else
+			c.dump = smc_diag_dump;
+		return netlink_dump_start(net->diag_nlsk, skb, h, &c);
 	}
 	return 0;
 }