linux-kernel - Re: [PATCH] cleanup block based fiemap

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.1004230753480.3739@i5.linux-foundation.org>
Date:	Fri, 23 Apr 2010 08:00:35 -0700 (PDT)
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, randy.dunlap@...cle.com
Subject: Re: [PATCH] cleanup block based fiemap



On Fri, 23 Apr 2010, Josef Bacik wrote:
> 
> I'm resending this patch again since it doesn't seem to have made it in yet.
> The generic block fiemap stuff doesn't use the right typing and has a problem
> with not setting the last extent flag properly.  Also there is an issue with
> GFS2 where it doesn't like non-block aligned requests, so this fixes all these
> issues.  Thanks,

I'd really like the patch to clean up the crazy stuff too.

As-is, there's at least two remaining issues I see from just reading the 
patch:

> +	if (len >= i_size_read(inode)) {
> +		whole_file = true;
> +		len = i_size_read(inode);
> +	}
...
>  			if (!past_eof &&
>  			    blk_to_logical(inode, start_blk) >=
> -			    blk_to_logical(inode, 0)+i_size_read(inode))
> +			    blk_to_logical(inode, 0) + i_size_read(inode))
>  				past_eof = 1;

Issue #1: it does that i_size_read() several times. What happens if the 
file grows? Maybe we hold the i_mutex already, although I don't see it. 
Regardless, it seems bogus to read the size several times.

Issue #2: "blk_to_logical(inode, 0)"? WTF? Since when has shifting zero 
ever resulted in anything interesting or relevant? There's at least two of 
those things.

		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/