Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] iomap: iomap_bmap should check iomap_apply return value
@ 2019-11-07  2:59 Darrick J. Wong
  2019-11-07  8:30 ` Christoph Hellwig
  0 siblings, 1 reply; 5+ messages in thread
From: Darrick J. Wong @ 2019-11-07  2:59 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs, linux-fsdevel

From: Darrick J. Wong <darrick.wong@oracle.com>

Check the return value of iomap_apply and return 0 (i.e. error) if it
didn't succeed.

Coverity-id: 1437065
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/iomap/fiemap.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
index 690ef2d7c6c8..bccf305ea9ce 100644
--- a/fs/iomap/fiemap.c
+++ b/fs/iomap/fiemap.c
@@ -133,12 +133,16 @@ iomap_bmap(struct address_space *mapping, sector_t bno,
 	struct inode *inode = mapping->host;
 	loff_t pos = bno << inode->i_blkbits;
 	unsigned blocksize = i_blocksize(inode);
+	int ret;
 
 	if (filemap_write_and_wait(mapping))
 		return 0;
 
 	bno = 0;
-	iomap_apply(inode, pos, blocksize, 0, ops, &bno, iomap_bmap_actor);
+	ret = iomap_apply(inode, pos, blocksize, 0, ops, &bno,
+			  iomap_bmap_actor);
+	if (ret)
+		return 0;
 	return bno;
 }
 EXPORT_SYMBOL_GPL(iomap_bmap);

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] iomap: iomap_bmap should check iomap_apply return value
  2019-11-07  2:59 [PATCH] iomap: iomap_bmap should check iomap_apply return value Darrick J. Wong
@ 2019-11-07  8:30 ` Christoph Hellwig
  2019-11-07 15:36   ` Darrick J. Wong
  0 siblings, 1 reply; 5+ messages in thread
From: Christoph Hellwig @ 2019-11-07  8:30 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Christoph Hellwig, linux-xfs, linux-fsdevel

On Wed, Nov 06, 2019 at 06:59:27PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Check the return value of iomap_apply and return 0 (i.e. error) if it
> didn't succeed.

And how could we set the bno value if we didn't succeed?

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] iomap: iomap_bmap should check iomap_apply return value
  2019-11-07  8:30 ` Christoph Hellwig
@ 2019-11-07 15:36   ` Darrick J. Wong
  2019-11-08  5:51     ` Christoph Hellwig
  0 siblings, 1 reply; 5+ messages in thread
From: Darrick J. Wong @ 2019-11-07 15:36 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs, linux-fsdevel

On Thu, Nov 07, 2019 at 09:30:50AM +0100, Christoph Hellwig wrote:
> On Wed, Nov 06, 2019 at 06:59:27PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Check the return value of iomap_apply and return 0 (i.e. error) if it
> > didn't succeed.
> 
> And how could we set the bno value if we didn't succeed?

The iomap_bmap caller supplies an ->iomap_end that returns an error.

Granted there's only one caller and it doesn't, so we could dump this
patch and just tell Coverity to shut up, but it's odd that this is the
one place where we ignore the return value.

OTOH it's bmap which has been broken for ages; the more insane behavior
seen in the wild, the better to scare away users. :P

--D

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] iomap: iomap_bmap should check iomap_apply return value
  2019-11-07 15:36   ` Darrick J. Wong
@ 2019-11-08  5:51     ` Christoph Hellwig
  2019-11-08 16:11       ` Darrick J. Wong
  0 siblings, 1 reply; 5+ messages in thread
From: Christoph Hellwig @ 2019-11-08  5:51 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Christoph Hellwig, linux-xfs, linux-fsdevel

On Thu, Nov 07, 2019 at 07:36:17AM -0800, Darrick J. Wong wrote:
> On Thu, Nov 07, 2019 at 09:30:50AM +0100, Christoph Hellwig wrote:
> > On Wed, Nov 06, 2019 at 06:59:27PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Check the return value of iomap_apply and return 0 (i.e. error) if it
> > > didn't succeed.
> > 
> > And how could we set the bno value if we didn't succeed?
> 
> The iomap_bmap caller supplies an ->iomap_end that returns an error.
> 
> Granted there's only one caller and it doesn't, so we could dump this
> patch and just tell Coverity to shut up, but it's odd that this is the
> one place where we ignore the return value.
> 
> OTOH it's bmap which has been broken for ages; the more insane behavior
> seen in the wild, the better to scare away users. :P

Oh well.  I guess the patch is fine, it just isn't really needed as-is.

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] iomap: iomap_bmap should check iomap_apply return value
  2019-11-08  5:51     ` Christoph Hellwig
@ 2019-11-08 16:11       ` Darrick J. Wong
  0 siblings, 0 replies; 5+ messages in thread
From: Darrick J. Wong @ 2019-11-08 16:11 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs, linux-fsdevel

On Fri, Nov 08, 2019 at 06:51:51AM +0100, Christoph Hellwig wrote:
> On Thu, Nov 07, 2019 at 07:36:17AM -0800, Darrick J. Wong wrote:
> > On Thu, Nov 07, 2019 at 09:30:50AM +0100, Christoph Hellwig wrote:
> > > On Wed, Nov 06, 2019 at 06:59:27PM -0800, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > Check the return value of iomap_apply and return 0 (i.e. error) if it
> > > > didn't succeed.
> > > 
> > > And how could we set the bno value if we didn't succeed?
> > 
> > The iomap_bmap caller supplies an ->iomap_end that returns an error.
> > 
> > Granted there's only one caller and it doesn't, so we could dump this
> > patch and just tell Coverity to shut up, but it's odd that this is the
> > one place where we ignore the return value.
> > 
> > OTOH it's bmap which has been broken for ages; the more insane behavior
> > seen in the wild, the better to scare away users. :P
> 
> Oh well.  I guess the patch is fine, it just isn't really needed as-is.

Thanks for the review. :)

--D

> Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-07  2:59 [PATCH] iomap: iomap_bmap should check iomap_apply return value Darrick J. Wong
2019-11-07  8:30 ` Christoph Hellwig
2019-11-07 15:36   ` Darrick J. Wong
2019-11-08  5:51     ` Christoph Hellwig
2019-11-08 16:11       ` Darrick J. Wong

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git