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:   Fri, 1 May 2020 14:32:20 -0700
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net,
        jiri@...nulli.us
Cc:     netdev@...r.kernel.org, kernel-team@...com
Subject: Re: [PATCH net-next v3 2/3] devlink: let kernel allocate region
 snapshot id



On 4/30/2020 10:57 AM, Jakub Kicinski 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.
> 
> 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
> 
> v3:
>  - send the notification only once snapshot creation completed.
> 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     |  7 +-
>  net/core/devlink.c                            | 69 +++++++++++++++----
>  .../drivers/net/netdevsim/devlink.sh          | 13 ++++
>  3 files changed, 74 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/networking/devlink/devlink-region.rst b/Documentation/networking/devlink/devlink-region.rst
> index 04e04d1ff627..daf35427fce1 100644
> --- a/Documentation/networking/devlink/devlink-region.rst
> +++ b/Documentation/networking/devlink/devlink-region.rst
> @@ -23,7 +23,9 @@ 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.
>  
>  example usage
>  -------------
> @@ -45,7 +47,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 92afb85bad89..4df947fb90d9 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -4082,10 +4082,41 @@ static int devlink_nl_cmd_region_del(struct sk_buff *skb,
>  	return 0;
>  }
>  
> +static int
> +devlink_nl_snapshot_id_notify(struct devlink *devlink, struct genl_info *info,
> +			      struct devlink_region *region, u32 snapshot_id)
> +{
> +	struct devlink_snapshot *snapshot;
> +	struct sk_buff *msg;
> +	int err;
> +
> +	snapshot = devlink_region_snapshot_get_by_id(region, snapshot_id);
> +	if (WARN_ON(!snapshot))
> +		return -EINVAL;
> +
> +	msg = devlink_nl_region_notify_build(region, snapshot,
> +					     DEVLINK_CMD_REGION_NEW,
> +					     info->snd_portid, info->snd_seq);
> +	err = PTR_ERR_OR_ZERO(msg);
> +	if (err)
> +		goto err_notify;
> +
> +	err = genlmsg_reply(msg, info);
> +	if (err)
> +		goto err_notify;
> +
> +	return 0;
> +
> +err_notify:
> +	devlink_region_snapshot_del(region, snapshot);
> +	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;
> @@ -4097,11 +4128,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) {
> @@ -4119,16 +4145,25 @@ 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_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;
> +		}
> +	}
>  
>  	err = region->ops->snapshot(devlink, info->extack, &data);
>  	if (err)
> @@ -4138,6 +4173,14 @@ devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
>  	if (err)
>  		goto err_snapshot_create;
>  
> +	if (!snapshot_id_attr) {
> +		/* destroys the snapshot on failure */
> +		err = devlink_nl_snapshot_id_notify(devlink, info,
> +						    region, snapshot_id);
> +		if (err)
> +			return err;
> +	}
> +

Does it make sense to report back the snapshot in all cases? I guess no,
since the caller knows the id, and it doesn't really simplify anything
to display this if you know the id already. Ok.

Thanks for this, I really like how this simplifies creating a snapshot!

-Jake

Powered by blists - more mailing lists