linux-kernel - Re: [PATCH] cleanup __generic_block_fiemap and fix a few minor issues

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: <alpine.LFD.2.00.1003040754150.7046@localhost.localdomain>
Date:	Thu, 4 Mar 2010 08:03:55 -0800 (PST)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Josef Bacik <josef@...hat.com>
cc:	akpm@...ux-foundation.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] cleanup __generic_block_fiemap and fix a few minor
 issues

On Wed, 3 Mar 2010, Josef Bacik wrote:
> 
> 1) Some filesystems (gfs2) do not like being given non-block-aligned b_size's to
> map, so make sure we block align the b_size.

As far as I can tell, that alignment is wrong.

You can't just align "len". If "start" wasn't aligned, then just aligning 
"len" is wrong. And if start is guaranteed to be aligned, how?

Also, the alignment itself is ugly as hell, doing

> +	len = (len + (1 << inode->i_blkbits) - 1) &
> +		~((1 << inode->i_blkbits) - 1);
> +	map_end = start + len;

When what you probably should have done is to do

	map_end = start + len;

_before_ rounding 'start', and then after that you can now map 'start' and 
'map_end' just using the regular "logical_to_blk" that you already use (up 
or down depending on whether the thing should be inclusive or exclusive of 
partial blocks). No?

IOW, the whole thing should probably look something like

	sector_t start_blk, last_blk;

	start_blk = logical_to_blk(inode, start);
	last_blk = logical_to_blk(inode, start + len - 1);

and you're done. Short, sweet, and to the point (note that "last_blk" 
really is the _last_ block, so if you want the length in blocks, it would 
be "last_blk - start_blk + 1").

			Linus
--
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/