linux-kernel - Re: [PATCH v5 04/11] blksnap: header file of the module interface

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: <6168e4d5-efc3-0c84-66c7-aea460c9fcaa@veeam.com>
Date:   Tue, 18 Jul 2023 11:53:54 +0200
From:   Sergei Shtepa <sergei.shtepa@...am.com>
To:     Thomas Weißschuh <thomas@...ch.de>
CC:     <axboe@...nel.dk>, <hch@...radead.org>, <corbet@....net>,
        <snitzer@...nel.org>, <viro@...iv.linux.org.uk>,
        <brauner@...nel.org>, <dchinner@...hat.com>, <willy@...radead.org>,
        <dlemoal@...nel.org>, <jack@...e.cz>, <ming.lei@...hat.com>,
        <linux-block@...r.kernel.org>, <linux-doc@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-fsdevel@...r.kernel.org>,
        Donald Buczek <buczek@...gen.mpg.de>
Subject: Re: [PATCH v5 04/11] blksnap: header file of the module interface

Hi!
Thanks for the review.

On 7/17/23 20:57, Thomas Weißschuh wrote:
> Subject:
> Re: [PATCH v5 04/11] blksnap: header file of the module interface
> From:
> Thomas Weißschuh <thomas@...ch.de>
> Date:
> 7/17/23, 20:57
> 
> To:
> Sergei Shtepa <sergei.shtepa@...am.com>
> CC:
> axboe@...nel.dk, hch@...radead.org, corbet@....net, snitzer@...nel.org, viro@...iv.linux.org.uk, brauner@...nel.org, dchinner@...hat.com, willy@...radead.org, dlemoal@...nel.org, jack@...e.cz, ming.lei@...hat.com, linux-block@...r.kernel.org, linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org, Donald Buczek <buczek@...gen.mpg.de>
> 
> 
> On 2023-06-12 15:52:21+0200, Sergei Shtepa wrote:
> 
>> [..]
>> diff --git a/include/uapi/linux/blksnap.h b/include/uapi/linux/blksnap.h
>> new file mode 100644
>> index 000000000000..2fe3f2a43bc5
>> --- /dev/null
>> +++ b/include/uapi/linux/blksnap.h
>> @@ -0,0 +1,421 @@
>> [..]
>> +/**
>> + * struct blksnap_snapshotinfo - Result for the command
>> + *	&blkfilter_ctl_blksnap.blkfilter_ctl_blksnap_snapshotinfo.
>> + *
>> + * @error_code:
>> + *	Zero if there were no errors while holding the snapshot.
>> + *	The error code -ENOSPC means that while holding the snapshot, a snapshot
>> + *	overflow situation has occurred. Other error codes mean other reasons
>> + *	for failure.
>> + *	The error code is reset when the device is added to a new snapshot.
>> + * @image:
>> + *	If the snapshot was taken, it stores the block device name of the
>> + *	image, or empty string otherwise.
>> + */
>> +struct blksnap_snapshotinfo {
>> +	__s32 error_code;
>> +	__u8 image[IMAGE_DISK_NAME_LEN];
> Nitpick:
> 
> Seems a bit weird to have a signed error code that is always negative.
> Couldn't this be an unsigned number or directly return the error from
> the ioctl() itself?

Yes, it's a good idea to pass the error code as an unsigned value.
And this positive value can be passed in case of successful execution
of ioctl(), but I would not like to put different error signs in one value.

> 
>> +};
>> +
>> +/**
>> + * DOC: Interface for managing snapshots
>> + *
>> + * Control commands that are transmitted through the blksnap module interface.
>> + */
>> +enum blksnap_ioctl {
>> +	blksnap_ioctl_version,
>> +	blksnap_ioctl_snapshot_create,
>> +	blksnap_ioctl_snapshot_destroy,
>> +	blksnap_ioctl_snapshot_append_storage,
>> +	blksnap_ioctl_snapshot_take,
>> +	blksnap_ioctl_snapshot_collect,
>> +	blksnap_ioctl_snapshot_wait_event,
>> +};
>> +
>> +/**
>> + * struct blksnap_version - Module version.
>> + *
>> + * @major:
>> + *	Version major part.
>> + * @minor:
>> + *	Version minor part.
>> + * @revision:
>> + *	Revision number.
>> + * @build:
>> + *	Build number. Should be zero.
>> + */
>> +struct blksnap_version {
>> +	__u16 major;
>> +	__u16 minor;
>> +	__u16 revision;
>> +	__u16 build;
>> +};
>> +
>> +/**
>> + * define IOCTL_BLKSNAP_VERSION - Get module version.
>> + *
>> + * The version may increase when the API changes. But linking the user space
>> + * behavior to the version code does not seem to be a good idea.
>> + * To ensure backward compatibility, API changes should be made by adding new
>> + * ioctl without changing the behavior of existing ones. The version should be
>> + * used for logs.
>> + *
>> + * Return: 0 if succeeded, negative errno otherwise.
>> + */
>> +#define IOCTL_BLKSNAP_VERSION							\
>> +	_IOW(BLKSNAP, blksnap_ioctl_version, struct blksnap_version)
> Shouldn't this be _IOR()?
> 
>   "_IOW means userland is writing and kernel is reading. _IOR
>   means userland is reading and kernel is writing."
> 
> The other ioctl definitions seem to need a review, too.
> 

Yeah. I need to replace _IOR and _IOW in all ioctl.
Thanks!