| 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
| ||
|
Message-ID: <20200203134423.GG2260@nanopsycho>
Date: Mon, 3 Feb 2020 14:44:23 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Jacob Keller <jacob.e.keller@...el.com>
Cc: netdev@...r.kernel.org, valex@...lanox.com, linyunsheng@...wei.com,
lihong.yang@...el.com
Subject: Re: [PATCH 13/15] devlink: support directly reading from region
memory
Thu, Jan 30, 2020 at 11:59:08PM CET, jacob.e.keller@...el.com wrote:
>Add a new region operation for directly reading from a region, without
>taking a full snapshot.
>
>Extend the DEVLINK_CMD_REGION_READ to allow directly reading from
>a region, if supported. Instead of reporting a missing snapshot id as
>invalid, check to see if direct reading is implemented for the region.
>If so, use the direct read operation to grab the current contents of the
>region.
>
>This new behavior of DEVLINK_CMD_REGION_READ should be backwards
>compatible. Previously, all kernels rejected such
>a DEVLINK_CMD_REGION_READ with -EINVAL, and will now either accept the
>call or report -EOPNOTSUPP for regions which do not implement direct
>access.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
>---
> .../networking/devlink/devlink-region.rst | 8 ++
> include/net/devlink.h | 4 +
> net/core/devlink.c | 82 +++++++++++++++++--
> 3 files changed, 87 insertions(+), 7 deletions(-)
>
>diff --git a/Documentation/networking/devlink/devlink-region.rst b/Documentation/networking/devlink/devlink-region.rst
>index 262249e6c3fc..a543f5ee7a9e 100644
>--- a/Documentation/networking/devlink/devlink-region.rst
>+++ b/Documentation/networking/devlink/devlink-region.rst
>@@ -25,6 +25,10 @@ Regions may optionally support capturing a snapshot on demand via the
> allow requested snapshots must implement the ``.snapshot`` callback for the
> region in its ``devlink_region_ops`` structure.
>
>+Regions may optionally allow directly reading from their contents without a
>+snapshot. A driver wishing to enable this for a region should implement the
>+``.read`` callback in the ``devlink_region_ops`` structure.
>+
> example usage
> -------------
>
>@@ -60,6 +64,10 @@ example usage
> length 16
> 0000000000000000 0014 95dc 0014 9514 0035 1670 0034 db30
>
>+ # Read from the region without a snapshot
>+ $ devlink region read pci/0000:00:05.0/fw-health address 16 length 16
>+ 0000000000000010 0000 0000 ffff ff04 0029 8c00 0028 8cc8
>+
> As regions are likely very device or driver specific, no generic regions are
> defined. See the driver-specific documentation files for information on the
> specific regions a driver supports.
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 1c3540280396..47ce1b5481de 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -508,6 +508,10 @@ struct devlink_region_ops {
> struct netlink_ext_ack *extack,
> u8 **data,
> devlink_snapshot_data_dest_t **destructor);
>+ int (*read)(struct devlink *devlink,
>+ u64 curr_offset,
>+ u32 data_size,
>+ u8 *data);
Too much wrapping.
> };
>
> struct devlink_fmsg;
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index b2b855d12a11..5831b7b78915 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -4005,6 +4005,56 @@ static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb,
> return err;
> }
>
>+static int devlink_nl_region_read_direct_fill(struct sk_buff *skb,
>+ struct devlink *devlink,
>+ struct devlink_region *region,
>+ struct nlattr **attrs,
>+ u64 start_offset,
>+ u64 end_offset,
>+ bool dump,
>+ u64 *new_offset)
Again.
>+{
>+ u64 curr_offset = start_offset;
>+ int err = 0;
>+ u8 *data;
>+
>+ /* Allocate and re-use a single buffer */
>+ data = kzalloc(DEVLINK_REGION_READ_CHUNK_SIZE, GFP_KERNEL);
>+ if (!data)
>+ return -ENOMEM;
>+
>+ *new_offset = start_offset;
>+
>+ if (end_offset > region->size || dump)
>+ end_offset = region->size;
>+
>+ while (curr_offset < end_offset) {
>+ u32 data_size;
>+
>+ if (end_offset - curr_offset < DEVLINK_REGION_READ_CHUNK_SIZE)
>+ data_size = end_offset - curr_offset;
>+ else
>+ data_size = DEVLINK_REGION_READ_CHUNK_SIZE;
>+
>+ err = region->ops->read(devlink, curr_offset, data_size, data);
There is a lot of code duplication is this function. Perhap there could
be a cb and cb_priv here to distinguish shapshot and direct read?
>+ if (err)
>+ break;
>+
>+ err = devlink_nl_cmd_region_read_chunk_fill(skb, devlink,
>+ data, data_size,
>+ curr_offset);
>+ if (err)
>+ break;
>+
>+ curr_offset += data_size;
>+ }
>+ *new_offset = curr_offset;
>+
>+ kfree(data);
>+
>+ return err;
>+}
>+
> static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> struct netlink_callback *cb)
> {
>@@ -4016,6 +4066,7 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> const char *region_name;
> struct devlink *devlink;
> bool dump = true;
>+ bool direct;
> void *hdr;
> int err;
>
>@@ -4030,8 +4081,7 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
>
> mutex_lock(&devlink->lock);
>
>- if (!attrs[DEVLINK_ATTR_REGION_NAME] ||
>- !attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) {
>+ if (!attrs[DEVLINK_ATTR_REGION_NAME]) {
> err = -EINVAL;
> goto out_unlock;
> }
>@@ -4043,6 +4093,17 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> goto out_unlock;
> }
>
>+ if (attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID])
>+ direct = false;
>+ else
>+ direct = true;
direct = !attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID];
>+
>+ /* Region may not support direct read access */
>+ if (direct && !region->ops->read) {
extack msg please.
>+ err = -EOPNOTSUPP;
>+ goto out_unlock;
>+ }
>+
> hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
> &devlink_nl_family, NLM_F_ACK | NLM_F_MULTI,
> DEVLINK_CMD_REGION_READ);
>@@ -4076,11 +4137,18 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> dump = false;
> }
>
>- err = devlink_nl_region_read_snapshot_fill(skb, devlink,
>- region, attrs,
>- start_offset,
>- end_offset, dump,
>- &ret_offset);
>+ if (direct)
>+ err = devlink_nl_region_read_direct_fill(skb, devlink,
>+ region, attrs,
>+ start_offset,
>+ end_offset, dump,
>+ &ret_offset);
>+ else
>+ err = devlink_nl_region_read_snapshot_fill(skb, devlink,
>+ region, attrs,
>+ start_offset,
>+ end_offset, dump,
>+ &ret_offset);
>
> if (err && err != -EMSGSIZE)
> goto nla_put_failure;
>--
>2.25.0.rc1
>
Powered by blists - more mailing lists