Re: [PATCH 10/33] iomap: add an iomap-based bmap implementation

On Fri, May 11, 2018 at 08:25:27AM +0200, Christoph Hellwig wrote:

On Thu, May 10, 2018 at 08:08:38AM -0700, Darrick J. Wong wrote:

quoted

quoted

quoted

quoted

+	sector_t *bno = data;
+
+	if (iomap->type == IOMAP_MAPPED)
+		*bno = (iomap->addr + pos - iomap->offset) >> inode->i_blkbits;

Does this need to be careful w.r.t. overflow on systems where sector_t
is a 32-bit unsigned long?

Also, ioctl_fibmap() typecasts the returned sector_t to an int, which
also seems broken.  I agree the interface needs to die, but ioctls take
a long time to deprecate.

Not much we can do about the interface.

Yes, the interface is fubar, but if file /foo maps to block 8589934720
then do we return the truncated result 128?

Then we'll get a corrupt result.  What do you think we could do here
eithere in the old or new code?

I think the only thing we /can/ do is figure out if we'd be truncating
the result, dump a warning to the kernel, and return 0, because we don't
want smartypants FIBMAP callers to be using crap block pointers.

Something like this for the bmap implementation...

uint64_t mapping = iomap->addr;

#ifdef CONFIG_LBDAF
if (mapping > ULONG_MAX) {
	/* Do not truncate results. */
	return 0;
}
#endif

...and in the bmap ioctl...

sector_t mapping = ...;

if (mapping > INT_MAX) {
	WARN(1, "would truncate bmap result, go fix your stupid program");
	return 0;
}

--D

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html