netdev - Re: [PATCH net-next v3 02/11] devlink: Add callback to query for snapshot id before snapshot create

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: <20180712175128.457848ec@cakuba.lan>
Date:   Thu, 12 Jul 2018 17:51:28 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Alex Vesker <valex@...lanox.com>
Cc:     netdev@...r.kernel.org, jiri@...lanox.com, dsahern@...il.com,
        andrew@...n.ch, rahul.lakkireddy@...lsio.com
Subject: Re: [PATCH net-next v3 02/11] devlink: Add callback to query for
 snapshot id before snapshot create

On Thu, 12 Jul 2018 15:13:09 +0300, Alex Vesker wrote:
> To restrict the driver with the snapshot ID selection a new callback
> is introduced for the driver to get the snapshot ID before creating
> a new snapshot. This will also allow giving the same ID for multiple
> snapshots taken of different regions on the same time.

I'm not in position to criticize other people's commit messages :), but
I find this one hard to parse.  I think what you meant to say is that
you add a helper for numbering the snapshot per-devlink instance.
There is no callback to be seen here.  You *prevent* from giving the
same ID to multiple snapshot even if they are from different regions.

> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index cac8561..6c92ddd 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -4193,6 +4193,27 @@ void devlink_region_destroy(struct devlink_region *region)
>  }
>  EXPORT_SYMBOL_GPL(devlink_region_destroy);
>  
> +/**
> + *	devlink_region_shapshot_id_get - get snapshot ID
> + *
> + *	This callback should be called when adding a new snapshot,
> + *	Driver should use the same id for multiple snapshots taken
> + *	on multiple regions at the same time/by the same trigger.
> + *
> + *	@devlink: devlink
> + */
> +u32 devlink_region_shapshot_id_get(struct devlink *devlink)
> +{
> +	u32 id;
> +
> +	mutex_lock(&devlink->lock);
> +	id = ++devlink->snapshot_id;

Any reason not to use an IDA?  The reuse may seem unlikely, OTOH IDA
isn't going to cost much, so why risk it...

> +	mutex_unlock(&devlink->lock);
> +
> +	return id;
> +}
> +EXPORT_SYMBOL_GPL(devlink_region_shapshot_id_get);

Sorry for only spotting this now.