netdev - Re: [PATCH 04/15] netdevsim: support taking immediate snapshot via devlink

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: <20200131100712.5ba1ce65@cakuba.hsd1.ca.comcast.net>
Date:   Fri, 31 Jan 2020 10:07:12 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jacob Keller <jacob.e.keller@...el.com>
Cc:     netdev@...r.kernel.org, jiri@...nulli.us, valex@...lanox.com,
        linyunsheng@...wei.com, lihong.yang@...el.com
Subject: Re: [PATCH 04/15] netdevsim: support taking immediate snapshot via
 devlink

On Thu, 30 Jan 2020 14:58:59 -0800, Jacob Keller wrote:
> Implement the .snapshot region operation for the dummy data region. This
> enables a region snapshot to be taken upon request via the new
> DEVLINK_CMD_REGION_SNAPSHOT command.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> ---
>  drivers/net/netdevsim/dev.c                   | 38 +++++++++++++++----
>  .../drivers/net/netdevsim/devlink.sh          |  5 +++
>  2 files changed, 36 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
> index d521b7bfe007..924cd328037f 100644
> --- a/drivers/net/netdevsim/dev.c
> +++ b/drivers/net/netdevsim/dev.c
> @@ -38,24 +38,47 @@ static struct dentry *nsim_dev_ddir;
>  
>  #define NSIM_DEV_DUMMY_REGION_SIZE (1024 * 32)
>  
> +static int nsim_dev_take_snapshot(struct devlink *devlink,

nit: break the line after static int, you've done it in other patches
    so I trust you agree it's a superior formatting style :)

> +				  struct netlink_ext_ack *extack,
> +				  u8 **data,
> +				  devlink_snapshot_data_dest_t **destructor)
> +{
> +	void *dummy_data;
> +
> +	dummy_data = kmalloc(NSIM_DEV_DUMMY_REGION_SIZE, GFP_KERNEL);
> +	if (!dummy_data) {
> +		NL_SET_ERR_MSG(extack, "Out of memory");

Unnecessary, there will be an OOM splat, and ENOMEM is basically
exactly the same as the message.

> +		return -ENOMEM;
> +	}
> +
> +	get_random_bytes(dummy_data, NSIM_DEV_DUMMY_REGION_SIZE);
> +
> +	*data = dummy_data;
> +	*destructor = kfree;

Is there any driver which uses different destructor for different
snapshots? Looks like something we could put in ops, maybe?

> +	return 0;
> +}
> +
>  static ssize_t nsim_dev_take_snapshot_write(struct file *file,
>  					    const char __user *data,
>  					    size_t count, loff_t *ppos)
>  {
>  	struct nsim_dev *nsim_dev = file->private_data;
> -	void *dummy_data;
> +	devlink_snapshot_data_dest_t *destructor;
> +	u8 *dummy_data;
>  	int err;
>  	u32 id;
>  
> -	dummy_data = kmalloc(NSIM_DEV_DUMMY_REGION_SIZE, GFP_KERNEL);
> -	if (!dummy_data)
> -		return -ENOMEM;
> -
> -	get_random_bytes(dummy_data, NSIM_DEV_DUMMY_REGION_SIZE);
> +	err = nsim_dev_take_snapshot(priv_to_devlink(nsim_dev), NULL,
> +				     &dummy_data, &destructor);
> +	if (err) {
> +		pr_err("Failed to capture region snapshot\n");

Also not a very useful message for netdevsim IMHO give the caller
clearly requested a snapshot and will get a more informative error 
from errno.

> +		return err;
> +	}
>  
>  	id = devlink_region_snapshot_id_get(priv_to_devlink(nsim_dev));
>  	err = devlink_region_snapshot_create(nsim_dev->dummy_region,
> -					     dummy_data, id, kfree);
> +					     dummy_data, id, destructor);
>  	if (err) {
>  		pr_err("Failed to create region snapshot\n");
>  		kfree(dummy_data);