netdev - Re: [PATCH net-next v2 5/9] devlink: refactor region_read_snapshot

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y381byfh6Oz6xKBD@nanopsycho>
Date:   Thu, 24 Nov 2022 10:12:15 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jacob Keller <jacob.e.keller@...el.com>
Cc:     netdev@...r.kernel.org, Jiri Pirko <jiri@...dia.com>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next v2 5/9] devlink: refactor
 region_read_snapshot_fill to use a callback function

Wed, Nov 23, 2022 at 09:38:30PM CET, jacob.e.keller@...el.com wrote:
>The devlink_nl_region_read_snapshot_fill is used to copy the contents of
>a snapshot into a message for reporting to userspace via the
>DEVLINK_CMG_REGION_READ netlink message.
>
>A future change is going to add support for directly reading from
>a region. Almost all of the logic for this new capability is identical.
>
>To help reduce code duplication and make this logic more generic,
>refactor the function to take a cb and cb_priv pointer for doing the
>actual copy.
>
>Add a devlink_region_snapshot_fill implementation that will simply copy
>the relevant chunk of the region. This does require allocating some
>storage for the chunk as opposed to simply passing the correct address
>forward to the devlink_nl_cmg_region_read_chunk_fill function.
>
>A future change to implement support for directly reading from a region
>without a snapshot will provide a separate implementation that calls the
>newly added devlink region operation.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
>---
>Changes since v1:
>* Use kmalloc instead of kzalloc
>* Don't combine data_size declaration and assignment
>* Fix the always_unused placement for devlink_region_snapshot_fill
>
> net/core/devlink.c | 44 +++++++++++++++++++++++++++++++++++---------
> 1 file changed, 35 insertions(+), 9 deletions(-)
>
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index bd7af0600405..729e2162a4db 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -6460,25 +6460,36 @@ static int devlink_nl_cmd_region_read_chunk_fill(struct sk_buff *msg,
> 
> #define DEVLINK_REGION_READ_CHUNK_SIZE 256
> 
>-static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb,
>-						struct devlink_snapshot *snapshot,
>-						u64 start_offset,
>-						u64 end_offset,
>-						u64 *new_offset)
>+typedef int devlink_chunk_fill_t(void *cb_priv, u8 *chunk, u32 chunk_size,
>+				 u64 curr_offset,
>+				 struct netlink_ext_ack *extack);
>+
>+static int
>+devlink_nl_region_read_fill(struct sk_buff *skb, devlink_chunk_fill_t *cb,
>+			    void *cb_priv, u64 start_offset, u64 end_offset,
>+			    u64 *new_offset, struct netlink_ext_ack *extack)
> {
> 	u64 curr_offset = start_offset;
> 	int err = 0;
>+	u8 *data;
>+
>+	/* Allocate and re-use a single buffer */
>+	data = kmalloc(DEVLINK_REGION_READ_CHUNK_SIZE, GFP_KERNEL);

Hmm, I tried to figure out how to do this without extra alloc and
memcpy, didn't find any nice solution :/

Reviewed-by: Jiri Pirko <jiri@...dia.com>

Btw, do you plan to extend this for write as well. It might be valuable
for debugging purposes to have that. I recall we discussed it in past.



>+	if (!data)
>+		return -ENOMEM;
> 
> 	*new_offset = start_offset;
> 
> 	while (curr_offset < end_offset) {
> 		u32 data_size;
>-		u8 *data;
> 
> 		data_size = min_t(u32, end_offset - curr_offset,
> 				  DEVLINK_REGION_READ_CHUNK_SIZE);
> 
>-		data = &snapshot->data[curr_offset];
>+		err = cb(cb_priv, data, data_size, curr_offset, extack);
>+		if (err)
>+			break;
>+
> 		err = devlink_nl_cmd_region_read_chunk_fill(skb, data, data_size, curr_offset);
> 		if (err)
> 			break;
>@@ -6487,9 +6498,23 @@ static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb,
> 	}
> 	*new_offset = curr_offset;
> 
>+	kfree(data);
>+
> 	return err;
> }
> 
>+static int
>+devlink_region_snapshot_fill(void *cb_priv, u8 *chunk, u32 chunk_size,
>+			     u64 curr_offset,
>+			     struct netlink_ext_ack __always_unused *extack)
>+{
>+	struct devlink_snapshot *snapshot = cb_priv;
>+
>+	memcpy(chunk, &snapshot->data[curr_offset], chunk_size);
>+
>+	return 0;
>+}
>+
> static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> 					     struct netlink_callback *cb)
> {
>@@ -6608,8 +6633,9 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> 		goto nla_put_failure;
> 	}
> 
>-	err = devlink_nl_region_read_snapshot_fill(skb, snapshot, start_offset,
>-						   end_offset, &ret_offset);
>+	err = devlink_nl_region_read_fill(skb, &devlink_region_snapshot_fill,
>+					  snapshot, start_offset, end_offset,
>+					  &ret_offset, cb->extack);
> 
> 	if (err && err != -EMSGSIZE)
> 		goto nla_put_failure;
>-- 
>2.38.1.420.g319605f8f00e
>