linux-kernel - Re: [PATCH v7 11/22] Replace ext2_clear_xip_target with dax_clear

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: <20140410141630.GH5727@linux.intel.com>
Date:	Thu, 10 Apr 2014 10:16:30 -0400
From:	Matthew Wilcox <willy@...ux.intel.com>
To:	Jan Kara <jack@...e.cz>
Cc:	Matthew Wilcox <matthew.r.wilcox@...el.com>,
	linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 11/22] Replace ext2_clear_xip_target with
 dax_clear_blocks

On Wed, Apr 09, 2014 at 11:46:44AM +0200, Jan Kara wrote:
>   Another day, some more review ;) Comments below.

I'm really grateful for all this review!  It's killing me, though ;-)

> > +int dax_clear_blocks(struct inode *inode, sector_t block, long size)
> > +{
> > +	struct block_device *bdev = inode->i_sb->s_bdev;
> > +	const struct block_device_operations *ops = bdev->bd_disk->fops;
> > +	sector_t sector = block << (inode->i_blkbits - 9);
> > +	unsigned long pfn;
> > +
> > +	might_sleep();
> > +	do {
> > +		void *addr;
> > +		long count = ops->direct_access(bdev, sector, &addr, &pfn,
> > +									size);
>   So do you assume blocksize == PAGE_SIZE here? If not, addr could be in
> the middle of the page AFAICT.

You're right.  Depending on how clear_page() is implemented, that
might go badly wrong.  Of course, both ext2 & ext4 require block_size
== PAGE_SIZE right now, so anything else is by definition untested.
I've been trying to keep DAX free from that assumption, but obviously
haven't caught all the places.

How does this look?

typedef long (*direct_access_t)(struct block_device *, sector_t, void **,
                                unsigned long *pfn, long size);

int dax_clear_blocks(struct inode *inode, sector_t block, long size)
{
        struct block_device *bdev = inode->i_sb->s_bdev;
        direct_access_t direct_access = bdev->bd_disk->fops->direct_access;
        sector_t sector = block << (inode->i_blkbits - 9);
        unsigned long pfn;

        might_sleep();
        do {
                void *addr;
                long count = direct_access(bdev, sector, &addr, &pfn, size);
                if (count < 0)
                        return count;
                while (count > 0) {
                        unsigned pgsz = PAGE_SIZE - offset_in_page(addr);
                        if (pgsz > count)
                                pgsz = count;
                        if (pgsz < PAGE_SIZE)
                                memset(addr, 0, pgsz);
                        else
                                clear_page(addr);
                        addr += pgsz;
                        size -= pgsz;
                        count -= pgsz;
                        sector += pgsz / 512;
                        cond_resched();
                }
        } while (size);

        return 0;
}
EXPORT_SYMBOL_GPL(dax_clear_blocks);

> >  	if (IS_DAX(inode)) {
> >  		/*
> > -		 * we need to clear the block
> > +		 * block must be initialised before we put it in the tree
> > +		 * so that it's not found by another thread before it's
> > +		 * initialised
> >  		 */
> > -		err = ext2_clear_xip_target (inode,
> > -			le32_to_cpu(chain[depth-1].key));
> > +		err = dax_clear_blocks(inode, le32_to_cpu(chain[depth-1].key),
> > +						count << inode->i_blkbits);
>   Umm 'count' looks wrong here. You want to clear only one block, don't
> you?

I think I got confused between ext2 and ext4 here.  I do want to clear
only one block.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/