linux-kernel - Re: [PATCH v6 07/11] blksnap: difference storage and chunk

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: <ZXGEB5M1+/2QMxqi@infradead.org>
Date:   Thu, 7 Dec 2023 00:36:23 -0800
From:   Christoph Hellwig <hch@...radead.org>
To:     Sergei Shtepa <sergei.shtepa@...ux.dev>
Cc:     axboe@...nel.dk, hch@...radead.org, corbet@....net,
        snitzer@...nel.org, mingo@...hat.com, peterz@...radead.org,
        juri.lelli@...hat.com, viro@...iv.linux.org.uk, brauner@...nel.org,
        linux-block@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        Sergei Shtepa <sergei.shtepa@...am.com>
Subject: Re: [PATCH v6 07/11] blksnap: difference storage and chunk

> +static inline bool unsupported_mode(const umode_t m)
> +{
> +	return (S_ISCHR(m) || S_ISFIFO(m) || S_ISSOCK(m));
> +}
> +
> +static inline bool unsupported_flags(const unsigned int flags)
> +{
> +	if (!(flags | O_RDWR)) {
> +		pr_err("Read and write access is required\n");
> +		return true;
> +	}
> +	if (!(flags | O_EXCL)) {
> +		pr_err("Exclusive access is required\n");
> +		return true;
> +	}

You probably want to positively check the allowed flags and types
to be more future proof.  I'd also drop these very easy to trigger
messages.

> +	if (S_ISBLK(file_inode(file)->i_mode)) {

Splitting the blk and regular file open path into separate helpers
would improve readability a lot I think.

> +		/*
> +		 * The block device is opened non-exclusively.
> +		 * It should be exclusive to open the file whose descriptor is
> +		 * passed to the module.
> +		 */
> +		bdev = blkdev_get_by_dev(dev_id,
> +					 BLK_OPEN_READ | BLK_OPEN_WRITE,
> +					 NULL, NULL);

Note that this will have some interesting interaction with the patches
from Jan to optionally disallow any other write for exclusively opened
block devices.  But given that right now we don't support using the
original device as backing store, this probably should become an
exclusive open anyway.