linux-kernel - Re: [PATCH] nvmet: add revalidation support to bdev and file backed namespaces

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] [day] [month] [year] [list]

Message-ID: <20200406080305.GA1329@technoir>
Date:   Mon, 6 Apr 2020 10:03:05 +0200
From:   Anthony Iliopoulos <ailiop@...e.com>
To:     Christoph Hellwig <hch@....de>
Cc:     Sagi Grimberg <sagi@...mberg.me>,
        Chaitanya Kulkarni <chaitanya.kulkarni@....com>,
        linux-nvme@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] nvmet: add revalidation support to bdev and file backed
 namespaces

On Fri, Apr 03, 2020 at 08:43:31AM +0200, Christoph Hellwig wrote:
> On Thu, Apr 02, 2020 at 09:30:52PM +0200, Anthony Iliopoulos wrote:
> > Add support for detecting capacity changes on nvmet blockdev and file
> > backed namespaces. This allows for emulating and testing online resizing
> > of nvme devices and filesystems on top.
> > 
> > Signed-off-by: Anthony Iliopoulos <ailiop@...e.com>
> 
> I vaguely remember seeing a very similar patch before, is this a repost?

Not a repost, but you're right, apparently there was a similar patch
posted before: 20191008122904.20438-1-m.malygin@...ro.com, which instead
triggers revalidation via configfs.

> > +void nvmet_bdev_ns_revalidate(struct nvmet_ns *ns)
> > +{
> > +	loff_t size;
> > +
> > +	size = i_size_read(ns->bdev->bd_inode);
> > +
> > +	if (ns->size != size)
> > +		ns->size = size;
> 
> This can be:
> 
> 	ns->size = i_size_read(ns->bdev->bd_inode);

Fixed.

> > +void nvmet_file_ns_revalidate(struct nvmet_ns *ns)
> > +{
> > +	struct kstat stat;
> > +
> > +	if (!ns->file)
> > +		return;
> 
> Shouldn't this always be non-NULL?

Right, this would be unset only during nvmet_ns_disable, and by that
time the ns is off the list, so identify should never see this being
non-NULL. Removed.

> > +
> > +	if (vfs_getattr(&ns->file->f_path,
> > +			&stat, STATX_SIZE, AT_STATX_FORCE_SYNC))
> > +		return;
> 
> Use up the full line:
> 
> 	if (vfs_getattr(&ns->file->f_path, &stat, STATX_SIZE,
> 			AT_STATX_FORCE_SYNC))

Fixed.

> Also shouldn't there be error handling?  If we can't stat the file
> the namespace is toast.

Indeed, I think it makes sense to fail identify at that point with
NVME_SC_INVALID_NS.

If you'd rather go with this patch instead of the configfs approach,
I'll post a v2 with the fixes, and some associated blktests that
Chaitanya requested.

Thank you all for the reviews!