linux-kernel - Re: [PATCH v11 08/21] dax,ext2: 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1063411139.10919.1413560742957.JavaMail.zimbra@efficios.com>
Date:	Fri, 17 Oct 2014 15:45:42 +0000 (UTC)
From:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:	Matthew Wilcox <willy@...ux.intel.com>
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 v11 08/21] dax,ext2: Replace ext2_clear_xip_target with
 dax_clear_blocks

----- Original Message -----
> From: "Matthew Wilcox" <willy@...ux.intel.com>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@...icios.com>
> Cc: "Matthew Wilcox" <matthew.r.wilcox@...el.com>, linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
> linux-kernel@...r.kernel.org
> Sent: Thursday, October 16, 2014 11:22:34 PM
> Subject: Re: [PATCH v11 08/21] dax,ext2: Replace ext2_clear_xip_target with dax_clear_blocks
> 
> On Thu, Oct 16, 2014 at 12:05:25PM +0200, Mathieu Desnoyers wrote:
> > > +int dax_clear_blocks(struct inode *inode, sector_t block, long size)
> > > +{
> > > +	struct block_device *bdev = inode->i_sb->s_bdev;
> > > +	sector_t sector = block << (inode->i_blkbits - 9);
> > 
> > Is there a define e.g. SECTOR_SHIFT rather than using this hardcoded "9"
> > value ?
> 
> Yeah ... in half a dozen drivers, so introducing them globally spews
> warnings about redefining macros.  The '9' and '512' are sprinkled all
> over the storage parts of the kernel, it's a complete flustercluck that
> I wasn't about to try to unscrew.

Fair enough.

> 
> > > +		while (count > 0) {
> > > +			unsigned pgsz = PAGE_SIZE - offset_in_page(addr);
> > 
> > unsigned -> unsigned int
> 
> Any particular reason?  Omitting it in some places helps stay within
> the 80-column limit without sacrificing readability.

It looks like FS code often uses "unsigned", so I'm not too concerned.

It's just that I'm used to the Linux core kernel style, which tend to
use "unsigned int".

> 
> > > +		}
> > > +	} while (size);
> > 
> > Just to stay on the safe side, can we do while (size > 0) ? Just in case
> > an unforeseen issue makes size negative, and gets us in a very long loop.
> 
> If size < 0, we should BUG, because that means we've zeroed more than
> we were asked to do, which is data corruption.
> 
> There's probably some other hardening we should do for this loop.
> For example, if 'count' is < 512, it can go into an infinite loop.
> 
>         do {
>                 void *addr;
>                 unsigned long pfn;
>                 long count;
> 
>                 count = bdev_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;
> 			BUG_ON(pgsz & 511);
>                         sector += pgsz / 512;
>                         cond_resched();
>                 }
> 		BUG_ON(size < 0);
>         } while (size);
> 
> I think that should do the job ... ?
> 

Yep. I love defensive programming, especially for filesystems. :)

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
--
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/