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  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]
Date:   Wed, 29 Apr 2020 15:34:30 +0000
From:   "Keller, Jacob E" <jacob.e.keller@...el.com>
To:     Jiri Pirko <jiri@...nulli.us>, Jakub Kicinski <kuba@...nel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "kernel-team@...com" <kernel-team@...com>
Subject: RE: [PATCH net-next] devlink: let kernel allocate region snapshot id



> -----Original Message-----
> From: netdev-owner@...r.kernel.org <netdev-owner@...r.kernel.org> On
> Behalf Of Jiri Pirko
> Sent: Tuesday, April 28, 2020 10:46 PM
> To: Jakub Kicinski <kuba@...nel.org>
> Cc: davem@...emloft.net; netdev@...r.kernel.org; kernel-team@...com;
> Keller, Jacob E <jacob.e.keller@...el.com>
> Subject: Re: [PATCH net-next] devlink: let kernel allocate region snapshot id
> 
> Wed, Apr 29, 2020 at 03:42:48AM 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.
> >

I did propose something like this originally, but....

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

... sending a message back makes this work better.

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

It seems like we could wait until at least after the region is captured.

> >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
> >
> >Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> >---
> >Jiri, this is what I had in mind of snapshots and the same
> >thing will come back for slice allocation.
> 
> Okay. Could you please send the userspace patch too in order to see the
> full picture?
> 

Yes, I'd like to see this as well.

> 
> >
> > net/core/devlink.c                            | 84 ++++++++++++++++---
> > .../drivers/net/netdevsim/devlink.sh          | 13 +++
> > 2 files changed, 84 insertions(+), 13 deletions(-)
> >
> >diff --git a/net/core/devlink.c b/net/core/devlink.c
> >index 1ec2e9fd8898..dad5d07dd4f8 100644
> >--- a/net/core/devlink.c
> >+++ b/net/core/devlink.c
> >@@ -4065,10 +4065,65 @@ 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,
> 
> No need to wrap here.
> 
> 
> >+				   "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 +4135,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 +4152,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;
> >+	}
> >
> > 	err = region->ops->snapshot(devlink, info->extack, &data);
> 
> How the output is going to looks like it this or any of the follow-up
> calls in this function are going to fail?
> 
> I guess it is going to be handled gracefully in the userspace app,
> right?
> 
> 

I'm wondering what the issue is with just waiting to send the snapshot id back until after this succeeds. Is it just easier to keep it near the allocation?

Thanks,
Jake

Powered by blists - more mailing lists