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: <20200430045315.GF6581@nanopsycho.orion>
Date:   Thu, 30 Apr 2020 06:53:15 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, kernel-team@...com,
        jacob.e.keller@...el.com
Subject: Re: [PATCH net-next v2] devlink: let kernel allocate region snapshot
 id

Thu, Apr 30, 2020 at 01:38:13AM CEST, kuba@...nel.org wrote:
>Currently users have to choose a free snapshot id before
>calling DEVLINK_CMD_REGION_NEW. This is potentially racy
>and inconvenient.
>
>Make the DEVLINK_ATTR_REGION_SNAPSHOT_ID optional and try
>to allocate id automatically. Send a message back to the
>caller with the snapshot info.
>
>The message carrying id gets sent immediately, but the
>allocation is only valid if the entire operation succeeded.
>This makes life easier, as sending the notification itself
>may fail.
>
>Example use:
>$ devlink region new netdevsim/netdevsim1/dummy
>netdevsim/netdevsim1/dummy: snapshot 1
>
>$ id=$(devlink -j region new netdevsim/netdevsim1/dummy | \
>       jq '.[][][][]')
>$ devlink region dump netdevsim/netdevsim1/dummy snapshot $id
>[...]
>$ devlink region del netdevsim/netdevsim1/dummy snapshot $id
>
>v2:
> - don't wrap the line containing extack;
> - add a few sentences to the docs.
>
>Signed-off-by: Jakub Kicinski <kuba@...nel.org>
>---
> .../networking/devlink/devlink-region.rst     | 10 ++-
> net/core/devlink.c                            | 83 ++++++++++++++++---
> .../drivers/net/netdevsim/devlink.sh          | 13 +++
> 3 files changed, 91 insertions(+), 15 deletions(-)
>
>diff --git a/Documentation/networking/devlink/devlink-region.rst b/Documentation/networking/devlink/devlink-region.rst
>index 04e04d1ff627..b163d09a3d0d 100644
>--- a/Documentation/networking/devlink/devlink-region.rst
>+++ b/Documentation/networking/devlink/devlink-region.rst
>@@ -23,7 +23,12 @@ states, but see also :doc:`devlink-health`
> Regions may optionally support capturing a snapshot on demand via the
> ``DEVLINK_CMD_REGION_NEW`` netlink message. A driver wishing to allow
> requested snapshots must implement the ``.snapshot`` callback for the region
>-in its ``devlink_region_ops`` structure.
>+in its ``devlink_region_ops`` structure. If snapshot id is not set in
>+the ``DEVLINK_CMD_REGION_NEW`` request kernel will allocate one and send
>+the snapshot information to user space. Note that receiving the snapshot
>+information does not guarantee that the snapshot creation completed
>+successfully, user space must check the status of the operation before
>+proceeding.
> 
> example usage
> -------------
>@@ -45,7 +50,8 @@ example usage
>     $ devlink region del pci/0000:00:05.0/cr-space snapshot 1
> 
>     # Request an immediate snapshot, if supported by the region
>-    $ devlink region new pci/0000:00:05.0/cr-space snapshot 5
>+    $ devlink region new pci/0000:00:05.0/cr-space
>+    pci/0000:00:05.0/cr-space: snapshot 5
> 
>     # Dump a snapshot:
>     $ devlink region dump pci/0000:00:05.0/fw-health snapshot 1
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 1ec2e9fd8898..82703be1032e 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -4065,10 +4065,64 @@ static int devlink_nl_cmd_region_del(struct sk_buff *skb,
> 	return 0;
> }
> 
>+static int
>+devlink_nl_alloc_snapshot_id(struct devlink *devlink, struct genl_info *info,
>+			     struct devlink_region *region, u32 *snapshot_id)
>+{
>+	struct sk_buff *msg;
>+	void *hdr;
>+	int err;
>+
>+	err = __devlink_region_snapshot_id_get(devlink, snapshot_id);
>+	if (err) {
>+		NL_SET_ERR_MSG_MOD(info->extack, "Failed to allocate a new snapshot id");
>+		return err;
>+	}
>+
>+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>+	if (!msg) {
>+		err = -ENOMEM;
>+		goto err_msg_alloc;
>+	}
>+
>+	hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
>+			  &devlink_nl_family, 0, DEVLINK_CMD_REGION_NEW);
>+	if (!hdr) {
>+		err = -EMSGSIZE;
>+		goto err_put_failure;
>+	}
>+	err = devlink_nl_put_handle(msg, devlink);
>+	if (err)
>+		goto err_attr_failure;
>+	err = nla_put_string(msg, DEVLINK_ATTR_REGION_NAME, region->ops->name);
>+	if (err)
>+		goto err_attr_failure;
>+	err = nla_put_u32(msg, DEVLINK_ATTR_REGION_SNAPSHOT_ID, *snapshot_id);
>+	if (err)
>+		goto err_attr_failure;
>+	genlmsg_end(msg, hdr);
>+
>+	err = genlmsg_reply(msg, info);
>+	if (err)
>+		goto err_reply;
>+
>+	return 0;
>+
>+err_attr_failure:
>+	genlmsg_cancel(msg, hdr);
>+err_put_failure:
>+	nlmsg_free(msg);
>+err_msg_alloc:
>+err_reply:
>+	__devlink_snapshot_id_decrement(devlink, *snapshot_id);
>+	return err;
>+}
>+
> static int
> devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
> {
> 	struct devlink *devlink = info->user_ptr[0];
>+	struct nlattr *snapshot_id_attr;
> 	struct devlink_region *region;
> 	const char *region_name;
> 	u32 snapshot_id;
>@@ -4080,11 +4134,6 @@ devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
> 		return -EINVAL;
> 	}
> 
>-	if (!info->attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) {
>-		NL_SET_ERR_MSG_MOD(info->extack, "No snapshot id provided");
>-		return -EINVAL;
>-	}
>-
> 	region_name = nla_data(info->attrs[DEVLINK_ATTR_REGION_NAME]);
> 	region = devlink_region_get_by_name(devlink, region_name);
> 	if (!region) {
>@@ -4102,16 +4151,24 @@ devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
> 		return -ENOSPC;
> 	}
> 
>-	snapshot_id = nla_get_u32(info->attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]);
>+	snapshot_id_attr = info->attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID];
>+	if (snapshot_id_attr) {
>+		snapshot_id = nla_get_u32(snapshot_id_attr);
> 
>-	if (devlink_region_snapshot_get_by_id(region, snapshot_id)) {
>-		NL_SET_ERR_MSG_MOD(info->extack, "The requested snapshot id is already in use");
>-		return -EEXIST;
>-	}
>+		if (devlink_region_snapshot_get_by_id(region, snapshot_id)) {
>+			NL_SET_ERR_MSG_MOD(info->extack, "The requested snapshot id is already in use");
>+			return -EEXIST;
>+		}
> 
>-	err = __devlink_snapshot_id_insert(devlink, snapshot_id);
>-	if (err)
>-		return err;
>+		err = __devlink_snapshot_id_insert(devlink, snapshot_id);
>+		if (err)
>+			return err;
>+	} else {
>+		err = devlink_nl_alloc_snapshot_id(devlink, info,
>+						   region, &snapshot_id);
>+		if (err)
>+			return err;
>+	}

Could you please send the snapshot id just before you return 0 in this
function, as you offered in v1? I think it would be great to do it like
that.

Thanks!


> 
> 	err = region->ops->snapshot(devlink, info->extack, &data);
> 	if (err)
>diff --git a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
>index 9f9741444549..ad539eccddcb 100755
>--- a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
>+++ b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
>@@ -151,6 +151,19 @@ regions_test()
> 
> 	check_region_snapshot_count dummy post-second-delete 2
> 
>+	sid=$(devlink -j region new $DL_HANDLE/dummy | jq '.[][][][]')
>+	check_err $? "Failed to create a new snapshot with id allocated by the kernel"
>+
>+	check_region_snapshot_count dummy post-first-request 3
>+
>+	devlink region dump $DL_HANDLE/dummy snapshot $sid >> /dev/null
>+	check_err $? "Failed to dump a snapshot with id allocated by the kernel"
>+
>+	devlink region del $DL_HANDLE/dummy snapshot $sid
>+	check_err $? "Failed to delete snapshot with id allocated by the kernel"
>+
>+	check_region_snapshot_count dummy post-first-request 2
>+
> 	log_test "regions test"
> }
> 
>-- 
>2.25.4
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ