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: <CO1PR11MB5089966656A01AF44BD6F17ED6139@CO1PR11MB5089.namprd11.prod.outlook.com>
Date:   Mon, 28 Nov 2022 18:27:42 +0000
From:   "Keller, Jacob E" <jacob.e.keller@...el.com>
To:     Jiri Pirko <jiri@...nulli.us>
CC:     "netdev@...r.kernel.org" <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



> -----Original Message-----
> From: Jiri Pirko <jiri@...nulli.us>
> Sent: Thursday, November 24, 2022 1:12 AM
> To: Keller, Jacob E <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 :/
> 

I also came up blank as well :( I can take another look when sending v3 with the other fixups.

> 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.
> 

We could extend to support writing. I haven't had a strong need yet, and I worry about the potential for abuse of such an interface. We've already seen examples of people (ab)using such interfaces for unclear purposes...

Thanks,
Jake