[PATCH 0/5] Refactor ioctl_fibmap() internal interface

[PATCH 0/5] Refactor ioctl_fibmap() internal interface

[Carlos Maiolino]

Hi,

This series refactor the internal structure of FIBMAP so that the filesystem can
properly report errors back to VFS, and also simplifies its usage by
standardizing all ->bmap() method usage via bmap() function.

The last patch is a bug fix for ioctl_fibmap() calls with negative block values.



This patchset is essentially a part of the original series reworking FIEMAP to
also be used for FIBMAP calls.

Due the fact the original series makes too many changes, I decided to split it
into smaller series, so they can be reviewed and applied individually, with
specific purposes, instead of changing everything in a single set. I believe
this makes the review process for this work easier too.

Cheers.

Carlos Maiolino (5):
  fs: Enable bmap() function to properly return errors
  cachefiles: drop direct usage of ->bmap method.
  ecryptfs: drop direct calls to ->bmap
  fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
  fibmap: Reject negative block numbers

 drivers/md/md-bitmap.c | 16 ++++++++++------
 fs/cachefiles/rdwr.c   | 27 ++++++++++++++-------------
 fs/ecryptfs/mmap.c     | 16 ++++++----------
 fs/f2fs/data.c         | 16 +++++++++++-----
 fs/inode.c             | 30 +++++++++++++++++-------------
 fs/ioctl.c             | 32 ++++++++++++++++++++++----------
 fs/jbd2/journal.c      | 22 +++++++++++++++-------
 include/linux/fs.h     |  9 ++++++++-
 mm/page_io.c           | 11 +++++++----
 9 files changed, 110 insertions(+), 69 deletions(-)

-- 
2.23.0

[PATCH 1/5] fs: Enable bmap() function to properly return errors

[Carlos Maiolino]

By now, bmap() will either return the physical block number related to
the requested file offset or 0 in case of error or the requested offset
maps into a hole.
This patch makes the needed changes to enable bmap() to proper return
errors, using the return value as an error return, and now, a pointer
must be passed to bmap() to be filled with the mapped physical block.

It will change the behavior of bmap() on return:

- negative value in case of error
- zero on success or map fell into a hole

In case of a hole, the *block will be zero too

Since this is a prep patch, by now, the only error return is -EINVAL if
->bmap doesn't exist.

Changelog:

	V6:
		- Fix bmap() doc function
			Reported-by: kbuild test robot <lkp@intel.com>
	V5:
		- Rebasing against 5.3 required changes to the f2fs
		  check_swap_activate() function

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 drivers/md/md-bitmap.c | 16 ++++++++++------
 fs/f2fs/data.c         | 16 +++++++++++-----
 fs/inode.c             | 30 +++++++++++++++++-------------
 fs/jbd2/journal.c      | 22 +++++++++++++++-------
 include/linux/fs.h     |  2 +-
 mm/page_io.c           | 11 +++++++----
 6 files changed, 61 insertions(+), 36 deletions(-)

diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
index b092c7b5282f..bfb7fddd4f1b 100644
--- a/drivers/md/md-bitmap.c
+++ b/drivers/md/md-bitmap.c
@@ -364,7 +364,7 @@ static int read_page(struct file *file, unsigned long index,
 	int ret = 0;
 	struct inode *inode = file_inode(file);
 	struct buffer_head *bh;
-	sector_t block;
+	sector_t block, blk_cur;
 
 	pr_debug("read bitmap file (%dB @ %llu)\n", (int)PAGE_SIZE,
 		 (unsigned long long)index << PAGE_SHIFT);
@@ -375,17 +375,21 @@ static int read_page(struct file *file, unsigned long index,
 		goto out;
 	}
 	attach_page_buffers(page, bh);
-	block = index << (PAGE_SHIFT - inode->i_blkbits);
+	blk_cur = index << (PAGE_SHIFT - inode->i_blkbits);
 	while (bh) {
+		block = blk_cur;
+
 		if (count == 0)
 			bh->b_blocknr = 0;
 		else {
-			bh->b_blocknr = bmap(inode, block);
-			if (bh->b_blocknr == 0) {
-				/* Cannot use this file! */
+			ret = bmap(inode, &block);
+			if (ret || !block) {
 				ret = -EINVAL;
+				bh->b_blocknr = 0;
 				goto out;
 			}
+
+			bh->b_blocknr = block;
 			bh->b_bdev = inode->i_sb->s_bdev;
 			if (count < (1<<inode->i_blkbits))
 				count = 0;
@@ -399,7 +403,7 @@ static int read_page(struct file *file, unsigned long index,
 			set_buffer_mapped(bh);
 			submit_bh(REQ_OP_READ, 0, bh);
 		}
-		block++;
+		blk_cur++;
 		bh = bh->b_this_page;
 	}
 	page->index = index;
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 5755e897a5f0..f949d3590027 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -3068,12 +3068,16 @@ static int check_swap_activate(struct file *swap_file, unsigned int max)
 	while ((probe_block + blocks_per_page) <= last_block && page_no < max) {
 		unsigned block_in_page;
 		sector_t first_block;
+		sector_t block = 0;
+		int	 err = 0;
 
 		cond_resched();
 
-		first_block = bmap(inode, probe_block);
-		if (first_block == 0)
+		block = probe_block;
+		err = bmap(inode, &block);
+		if (err || !block)
 			goto bad_bmap;
+		first_block = block;
 
 		/*
 		 * It must be PAGE_SIZE aligned on-disk
@@ -3085,11 +3089,13 @@ static int check_swap_activate(struct file *swap_file, unsigned int max)
 
 		for (block_in_page = 1; block_in_page < blocks_per_page;
 					block_in_page++) {
-			sector_t block;
 
-			block = bmap(inode, probe_block + block_in_page);
-			if (block == 0)
+			block = probe_block + block_in_page;
+			err = bmap(inode, &block);
+
+			if (err || !block)
 				goto bad_bmap;
+
 			if (block != first_block + block_in_page) {
 				/* Discontiguity */
 				probe_block++;
diff --git a/fs/inode.c b/fs/inode.c
index fef457a42882..0a20aaa572f2 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1593,21 +1593,25 @@ EXPORT_SYMBOL(iput);
 
 /**
  *	bmap	- find a block number in a file
- *	@inode: inode of file
- *	@block: block to find
- *
- *	Returns the block number on the device holding the inode that
- *	is the disk block number for the block of the file requested.
- *	That is, asked for block 4 of inode 1 the function will return the
- *	disk block relative to the disk start that holds that block of the
- *	file.
+ *	@inode:  inode owning the block number being requested
+ *	@block: pointer containing the block to find
+ *
+ *	Replaces the value in *block with the block number on the device holding
+ *	corresponding to the requested block number in the file.
+ *	That is, asked for block 4 of inode 1 the function will replace the
+ *	4 in *block, with disk block relative to the disk start that holds that
+ *	block of the file.
+ *
+ *	Returns -EINVAL in case of error, 0 otherwise. If mapping falls into a
+ *	hole, returns 0 and *block is also set to 0.
  */
-sector_t bmap(struct inode *inode, sector_t block)
+int bmap(struct inode *inode, sector_t *block)
 {
-	sector_t res = 0;
-	if (inode->i_mapping->a_ops->bmap)
-		res = inode->i_mapping->a_ops->bmap(inode->i_mapping, block);
-	return res;
+	if (!inode->i_mapping->a_ops->bmap)
+		return -EINVAL;
+
+	*block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block);
+	return 0;
 }
 EXPORT_SYMBOL(bmap);
 
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 1c58859aa592..5664101c73a8 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -795,18 +795,23 @@ int jbd2_journal_bmap(journal_t *journal, unsigned long blocknr,
 {
 	int err = 0;
 	unsigned long long ret;
+	sector_t block = 0;
 
 	if (journal->j_inode) {
-		ret = bmap(journal->j_inode, blocknr);
-		if (ret)
-			*retp = ret;
-		else {
+		block = blocknr;
+		ret = bmap(journal->j_inode, &block);
+
+		if (ret || !block) {
 			printk(KERN_ALERT "%s: journal block not found "
 					"at offset %lu on %s\n",
 			       __func__, blocknr, journal->j_devname);
 			err = -EIO;
 			__journal_abort_soft(journal, err);
+
+		} else {
+			*retp = block;
 		}
+
 	} else {
 		*retp = blocknr; /* +journal->j_blk_offset */
 	}
@@ -1232,11 +1237,14 @@ journal_t *jbd2_journal_init_dev(struct block_device *bdev,
 journal_t *jbd2_journal_init_inode(struct inode *inode)
 {
 	journal_t *journal;
+	sector_t blocknr;
 	char *p;
-	unsigned long long blocknr;
+	int err = 0;
+
+	blocknr = 0;
+	err = bmap(inode, &blocknr);
 
-	blocknr = bmap(inode, 0);
-	if (!blocknr) {
+	if (err || !blocknr) {
 		pr_err("%s: Cannot locate journal superblock\n",
 			__func__);
 		return NULL;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e0d909d35763..8b31075415ce 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2863,7 +2863,7 @@ static inline ssize_t generic_write_sync(struct kiocb *iocb, ssize_t count)
 extern void emergency_sync(void);
 extern void emergency_remount(void);
 #ifdef CONFIG_BLOCK
-extern sector_t bmap(struct inode *, sector_t);
+extern int bmap(struct inode *, sector_t *);
 #endif
 extern int notify_change(struct dentry *, struct iattr *, struct inode **);
 extern int inode_permission(struct inode *, int);
diff --git a/mm/page_io.c b/mm/page_io.c
index 60a66a58b9bf..be663b504c80 100644
--- a/mm/page_io.c
+++ b/mm/page_io.c
@@ -176,8 +176,9 @@ int generic_swapfile_activate(struct swap_info_struct *sis,
 
 		cond_resched();
 
-		first_block = bmap(inode, probe_block);
-		if (first_block == 0)
+		first_block = probe_block;
+		ret = bmap(inode, &first_block);
+		if (ret || !first_block)
 			goto bad_bmap;
 
 		/*
@@ -192,9 +193,11 @@ int generic_swapfile_activate(struct swap_info_struct *sis,
 					block_in_page++) {
 			sector_t block;
 
-			block = bmap(inode, probe_block + block_in_page);
-			if (block == 0)
+			block = probe_block + block_in_page;
+			ret = bmap(inode, &block);
+			if (ret || !block)
 				goto bad_bmap;
+
 			if (block != first_block + block_in_page) {
 				/* Discontiguity */
 				probe_block++;
-- 
2.23.0

Re: [PATCH 1/5] fs: Enable bmap() function to properly return errors

[Christoph Hellwig]

On Fri, Nov 22, 2019 at 09:53:16AM +0100, Carlos Maiolino wrote:
> By now, bmap() will either return the physical block number related to
> the requested file offset or 0 in case of error or the requested offset
> maps into a hole.
> This patch makes the needed changes to enable bmap() to proper return
> errors, using the return value as an error return, and now, a pointer
> must be passed to bmap() to be filled with the mapped physical block.
> 
> It will change the behavior of bmap() on return:
> 
> - negative value in case of error
> - zero on success or map fell into a hole
> 
> In case of a hole, the *block will be zero too
> 
> Since this is a prep patch, by now, the only error return is -EINVAL if
> ->bmap doesn't exist.
> 
> Changelog:
> 
> 	V6:
> 		- Fix bmap() doc function
> 			Reported-by: kbuild test robot <lkp@intel.com>
> 	V5:
> 		- Rebasing against 5.3 required changes to the f2fs
> 		  check_swap_activate() function
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---

The changelog goes under the --- if you really want a per-patch
changelog.  I personally find the per-patch changelog horribly
distracting and much prefer just one in the cover letter, though.

Otherwise this looks good, although we really need to kill these
users rather sooner than later..

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

Re: [PATCH 1/5] fs: Enable bmap() function to properly return errors

[Carlos Maiolino]

On Fri, Nov 22, 2019 at 02:37:01PM +0100, Christoph Hellwig wrote:
> On Fri, Nov 22, 2019 at 09:53:16AM +0100, Carlos Maiolino wrote:
> > By now, bmap() will either return the physical block number related to
> > the requested file offset or 0 in case of error or the requested offset
> > maps into a hole.
> > This patch makes the needed changes to enable bmap() to proper return
> > errors, using the return value as an error return, and now, a pointer
> > must be passed to bmap() to be filled with the mapped physical block.
> > 
> > It will change the behavior of bmap() on return:
> > 
> > - negative value in case of error
> > - zero on success or map fell into a hole
> > 
> > In case of a hole, the *block will be zero too
> > 
> > Since this is a prep patch, by now, the only error return is -EINVAL if
> > ->bmap doesn't exist.
> > 
> > Changelog:
> > 
> > 	V6:
> > 		- Fix bmap() doc function
> > 			Reported-by: kbuild test robot <lkp@intel.com>
> > 	V5:
> > 		- Rebasing against 5.3 required changes to the f2fs
> > 		  check_swap_activate() function
> > 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> 
> The changelog goes under the --- if you really want a per-patch
> changelog.  I personally find the per-patch changelog horribly
> distracting and much prefer just one in the cover letter, though.
> 
> Otherwise this looks good, although we really need to kill these
> users rather sooner than later..
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 

Yeah, I forgot to move the changelogs under --- while formatting the patches, I
didn't mean to send them on the commit log.

This signed-off-by was meant to be a reviewed-by? I'm not sure if I got what you
meant with the sign-off here.


-- 
Carlos

Re: [PATCH 1/5] fs: Enable bmap() function to properly return errors

[Christoph Hellwig]

> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > 
> 
> Yeah, I forgot to move the changelogs under --- while formatting the patches, I
> didn't mean to send them on the commit log.
> 
> This signed-off-by was meant to be a reviewed-by? I'm not sure if I got what you
> meant with the sign-off here.

Yes, sorry.  Same for any other patch in case I messed that up.

[PATCH 2/5] cachefiles: drop direct usage of ->bmap method.

[Carlos Maiolino]

Replace the direct usage of ->bmap method by a bmap() call.

Changelog:
	V6:
		- ASSERT only if filesystem does not support bmap() to
		  match the original logic
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/cachefiles/rdwr.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/fs/cachefiles/rdwr.c b/fs/cachefiles/rdwr.c
index 44a3ce1e4ce4..1dc97f2d6201 100644
--- a/fs/cachefiles/rdwr.c
+++ b/fs/cachefiles/rdwr.c
@@ -396,7 +396,7 @@ int cachefiles_read_or_alloc_page(struct fscache_retrieval *op,
 	struct cachefiles_object *object;
 	struct cachefiles_cache *cache;
 	struct inode *inode;
-	sector_t block0, block;
+	sector_t block;
 	unsigned shift;
 	int ret;
 
@@ -412,7 +412,6 @@ int cachefiles_read_or_alloc_page(struct fscache_retrieval *op,
 
 	inode = d_backing_inode(object->backer);
 	ASSERT(S_ISREG(inode->i_mode));
-	ASSERT(inode->i_mapping->a_ops->bmap);
 	ASSERT(inode->i_mapping->a_ops->readpages);
 
 	/* calculate the shift required to use bmap */
@@ -428,12 +427,14 @@ int cachefiles_read_or_alloc_page(struct fscache_retrieval *op,
 	 *   enough for this as it doesn't indicate errors, but it's all we've
 	 *   got for the moment
 	 */
-	block0 = page->index;
-	block0 <<= shift;
+	block = page->index;
+	block <<= shift;
+
+	ret = bmap(inode, &block);
+	ASSERT(ret < 0);
 
-	block = inode->i_mapping->a_ops->bmap(inode->i_mapping, block0);
 	_debug("%llx -> %llx",
-	       (unsigned long long) block0,
+	       (unsigned long long) (page->index << shift),
 	       (unsigned long long) block);
 
 	if (block) {
@@ -711,7 +712,6 @@ int cachefiles_read_or_alloc_pages(struct fscache_retrieval *op,
 
 	inode = d_backing_inode(object->backer);
 	ASSERT(S_ISREG(inode->i_mode));
-	ASSERT(inode->i_mapping->a_ops->bmap);
 	ASSERT(inode->i_mapping->a_ops->readpages);
 
 	/* calculate the shift required to use bmap */
@@ -728,7 +728,7 @@ int cachefiles_read_or_alloc_pages(struct fscache_retrieval *op,
 
 	ret = space ? -ENODATA : -ENOBUFS;
 	list_for_each_entry_safe(page, _n, pages, lru) {
-		sector_t block0, block;
+		sector_t block;
 
 		/* we assume the absence or presence of the first block is a
 		 * good enough indication for the page as a whole
@@ -736,13 +736,14 @@ int cachefiles_read_or_alloc_pages(struct fscache_retrieval *op,
 		 *   good enough for this as it doesn't indicate errors, but
 		 *   it's all we've got for the moment
 		 */
-		block0 = page->index;
-		block0 <<= shift;
+		block = page->index;
+		block <<= shift;
+
+		ret = bmap(inode, &block);
+		ASSERT(!ret);
 
-		block = inode->i_mapping->a_ops->bmap(inode->i_mapping,
-						      block0);
 		_debug("%llx -> %llx",
-		       (unsigned long long) block0,
+		       (unsigned long long) (page->index << shift),
 		       (unsigned long long) block);
 
 		if (block) {
-- 
2.23.0

Re: [PATCH 2/5] cachefiles: drop direct usage of ->bmap method.

[Christoph Hellwig]

Looks good modulo the changelog placement.

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

Re: [PATCH 2/5] cachefiles: drop direct usage of ->bmap method.

[Carlos Maiolino]

On Fri, Nov 22, 2019 at 02:37:26PM +0100, Christoph Hellwig wrote:
> Looks good modulo the changelog placement.
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

I meant to move all changelogs to under '---' as I did on the previous versions,
I forgot to move them this time.

Do you want me to re-send, ripping out the changelogs and keeping your
Reviewed-by?


> 

-- 
Carlos

[PATCH 3/5] ecryptfs: drop direct calls to ->bmap

[Carlos Maiolino]

Replace direct ->bmap calls by bmap() method.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/ecryptfs/mmap.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c
index cffa0c1ec829..019572c6b39a 100644
--- a/fs/ecryptfs/mmap.c
+++ b/fs/ecryptfs/mmap.c
@@ -524,16 +524,12 @@ static int ecryptfs_write_end(struct file *file,
 
 static sector_t ecryptfs_bmap(struct address_space *mapping, sector_t block)
 {
-	int rc = 0;
-	struct inode *inode;
-	struct inode *lower_inode;
-
-	inode = (struct inode *)mapping->host;
-	lower_inode = ecryptfs_inode_to_lower(inode);
-	if (lower_inode->i_mapping->a_ops->bmap)
-		rc = lower_inode->i_mapping->a_ops->bmap(lower_inode->i_mapping,
-							 block);
-	return rc;
+	struct inode *lower_inode = ecryptfs_inode_to_lower(mapping->host);
+	int ret = bmap(lower_inode, &block);
+
+	if (ret)
+		return 0;
+	return block;
 }
 
 const struct address_space_operations ecryptfs_aops = {
-- 
2.23.0

[PATCH 4/5] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap

[Carlos Maiolino]

Now we have the possibility of proper error return in bmap, use bmap()
function in ioctl_fibmap() instead of calling ->bmap method directly.

V6:
	- Add a dummy bmap() definition so build does not break if
	  CONFIG_BLOCK is not set
		Reported-by: kbuild test robot <lkp@intel.com>
V4:
	- Ensure ioctl_fibmap() returns 0 in case of error returned from
	  bmap(). Otherwise we'll be changing the user interface (which
	  returns 0 in case of error)
V3:
	- Rename usr_blk to ur_block

V2:
	- Use a local sector_t variable to asign the block number
	  instead of using direct casting.
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/ioctl.c         | 29 +++++++++++++++++++----------
 include/linux/fs.h |  7 +++++++
 2 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index fef3a6bf7c78..6b589c873bc2 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -53,19 +53,28 @@ EXPORT_SYMBOL(vfs_ioctl);
 
 static int ioctl_fibmap(struct file *filp, int __user *p)
 {
-	struct address_space *mapping = filp->f_mapping;
-	int res, block;
+	struct inode *inode = file_inode(filp);
+	int error, ur_block;
+	sector_t block;
 
-	/* do we support this mess? */
-	if (!mapping->a_ops->bmap)
-		return -EINVAL;
 	if (!capable(CAP_SYS_RAWIO))
 		return -EPERM;
-	res = get_user(block, p);
-	if (res)
-		return res;
-	res = mapping->a_ops->bmap(mapping, block);
-	return put_user(res, p);
+
+	error = get_user(ur_block, p);
+	if (error)
+		return error;
+
+	block = ur_block;
+	error = bmap(inode, &block);
+
+	if (error)
+		ur_block = 0;
+	else
+		ur_block = block;
+
+	error = put_user(ur_block, p);
+
+	return error;
 }
 
 /**
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 8b31075415ce..8e0e56bc4b0c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2862,9 +2862,16 @@ static inline ssize_t generic_write_sync(struct kiocb *iocb, ssize_t count)
 
 extern void emergency_sync(void);
 extern void emergency_remount(void);
+
 #ifdef CONFIG_BLOCK
 extern int bmap(struct inode *, sector_t *);
+#else
+static inline int bmap(struct inode *,  sector_t *)
+{
+	return -EINVAL;
+}
 #endif
+
 extern int notify_change(struct dentry *, struct iattr *, struct inode **);
 extern int inode_permission(struct inode *, int);
 extern int generic_permission(struct inode *, int);
-- 
2.23.0

Re: [PATCH 4/5] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap

[Christoph Hellwig]

Looks good (modulo the changelog):

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

Re: [PATCH 4/5] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2144 bytes --]

Hi Carlos,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on hch-configfs/for-next]
[also build test ERROR on v5.4-rc8 next-20191122]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Carlos-Maiolino/Refactor-ioctl_fibmap-internal-interface/20191124-165458
base:   git://git.infradead.org/users/hch/configfs.git for-next
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-14) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/cgroup.h:17:0,
                    from include/linux/memcontrol.h:13,
                    from include/linux/swap.h:9,
                    from include/linux/suspend.h:5,
                    from arch/x86/kernel/asm-offsets.c:13:
   include/linux/fs.h: In function 'bmap':
>> include/linux/fs.h:2869:31: error: parameter name omitted
    static inline int bmap(struct inode *,  sector_t *)
                                  ^~~~~
>> include/linux/fs.h:2869:31: error: parameter name omitted
   make[2]: *** [arch/x86/kernel/asm-offsets.s] Error 1
   make[2]: Target '__build' not remade because of errors.
   make[1]: *** [prepare0] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [sub-make] Error 2
   29 real  15 user  17 sys  113.72% cpu 	make prepare

vim +2869 include/linux/fs.h

  2865	
  2866	#ifdef CONFIG_BLOCK
  2867	extern int bmap(struct inode *, sector_t *);
  2868	#else
> 2869	static inline int bmap(struct inode *,  sector_t *)
  2870	{
  2871		return -EINVAL;
  2872	}
  2873	#endif
  2874	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 7207 bytes --]

Re: [PATCH 4/5] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4177 bytes --]

Hi Carlos,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on hch-configfs/for-next]
[also build test ERROR on v5.4-rc8 next-20191122]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Carlos-Maiolino/Refactor-ioctl_fibmap-internal-interface/20191124-165458
base:   git://git.infradead.org/users/hch/configfs.git for-next
config: riscv-allnoconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=riscv 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from fs/inode.c:7:0:
   include/linux/fs.h: In function 'bmap':
   include/linux/fs.h:2869:31: error: parameter name omitted
    static inline int bmap(struct inode *,  sector_t *)
                                  ^~~~~
   include/linux/fs.h:2869:31: error: parameter name omitted
   fs/inode.c: At top level:
>> fs/inode.c:1608:5: error: redefinition of 'bmap'
    int bmap(struct inode *inode, sector_t *block)
        ^~~~
   In file included from fs/inode.c:7:0:
   include/linux/fs.h:2869:19: note: previous definition of 'bmap' was here
    static inline int bmap(struct inode *,  sector_t *)
                      ^~~~

vim +/bmap +1608 fs/inode.c

^1da177e4c3f41 Linus Torvalds  2005-04-16  1593  
^1da177e4c3f41 Linus Torvalds  2005-04-16  1594  /**
^1da177e4c3f41 Linus Torvalds  2005-04-16  1595   *	bmap	- find a block number in a file
1dc338a4af8188 Carlos Maiolino 2019-11-22  1596   *	@inode:  inode owning the block number being requested
1dc338a4af8188 Carlos Maiolino 2019-11-22  1597   *	@block: pointer containing the block to find
^1da177e4c3f41 Linus Torvalds  2005-04-16  1598   *
1dc338a4af8188 Carlos Maiolino 2019-11-22  1599   *	Replaces the value in *block with the block number on the device holding
1dc338a4af8188 Carlos Maiolino 2019-11-22  1600   *	corresponding to the requested block number in the file.
1dc338a4af8188 Carlos Maiolino 2019-11-22  1601   *	That is, asked for block 4 of inode 1 the function will replace the
1dc338a4af8188 Carlos Maiolino 2019-11-22  1602   *	4 in *block, with disk block relative to the disk start that holds that
1dc338a4af8188 Carlos Maiolino 2019-11-22  1603   *	block of the file.
1dc338a4af8188 Carlos Maiolino 2019-11-22  1604   *
1dc338a4af8188 Carlos Maiolino 2019-11-22  1605   *	Returns -EINVAL in case of error, 0 otherwise. If mapping falls into a
1dc338a4af8188 Carlos Maiolino 2019-11-22  1606   *	hole, returns 0 and *block is also set to 0.
^1da177e4c3f41 Linus Torvalds  2005-04-16  1607   */
1dc338a4af8188 Carlos Maiolino 2019-11-22 @1608  int bmap(struct inode *inode, sector_t *block)
^1da177e4c3f41 Linus Torvalds  2005-04-16  1609  {
1dc338a4af8188 Carlos Maiolino 2019-11-22  1610  	if (!inode->i_mapping->a_ops->bmap)
1dc338a4af8188 Carlos Maiolino 2019-11-22  1611  		return -EINVAL;
1dc338a4af8188 Carlos Maiolino 2019-11-22  1612  
1dc338a4af8188 Carlos Maiolino 2019-11-22  1613  	*block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block);
1dc338a4af8188 Carlos Maiolino 2019-11-22  1614  	return 0;
^1da177e4c3f41 Linus Torvalds  2005-04-16  1615  }
^1da177e4c3f41 Linus Torvalds  2005-04-16  1616  EXPORT_SYMBOL(bmap);
^1da177e4c3f41 Linus Torvalds  2005-04-16  1617  

:::::: The code at line 1608 was first introduced by commit
:::::: 1dc338a4af818855d7878307b8026c6af9e6304a fs: Enable bmap() function to properly return errors

:::::: TO: Carlos Maiolino <cmaiolino@redhat.com>
:::::: CC: 0day robot <lkp@intel.com>

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 5379 bytes --]

[PATCH 5/5] fibmap: Reject negative block numbers

[Carlos Maiolino]

FIBMAP receives an integer from userspace which is then implicitly converted
into sector_t to be passed to bmap(). No check is made to ensure userspace
didn't send a negative block number, which can end up in an underflow, and
returning to userspace a corrupted block address.

As a side-effect, the underflow caused by a negative block here, will
trigger the WARN() in iomap_bmap_actor(), which is how this issue was
first discovered.

This is essentially a V2 of a patch I sent a while ago, reworded and
refactored to fit into this patchset.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 6b589c873bc2..6b365b3eff70 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -64,6 +64,9 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
 	if (error)
 		return error;
 
+	if (ur_block < 0)
+		return -EINVAL;
+
 	block = ur_block;
 	error = bmap(inode, &block);
 
-- 
2.23.0

Re: [PATCH 5/5] fibmap: Reject negative block numbers

[Christoph Hellwig]

On Fri, Nov 22, 2019 at 09:53:20AM +0100, Carlos Maiolino wrote:
> FIBMAP receives an integer from userspace which is then implicitly converted
> into sector_t to be passed to bmap(). No check is made to ensure userspace
> didn't send a negative block number, which can end up in an underflow, and
> returning to userspace a corrupted block address.
> 
> As a side-effect, the underflow caused by a negative block here, will
> trigger the WARN() in iomap_bmap_actor(), which is how this issue was
> first discovered.
> 
> This is essentially a V2 of a patch I sent a while ago, reworded and
> refactored to fit into this patchset.

That last sentence should probably be removed.

Otherwise:

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

[PATCH 0/5] Refactor ioctl_fibmap() internal interface

[Carlos Maiolino]

Hi,

This series refactor the internal structure of FIBMAP so that the filesystem can
properly report errors back to VFS, and also simplifies its usage by
standardizing all ->bmap() method usage via bmap() function.

The last patch is a bug fix for ioctl_fibmap() calls with negative block values.

This new version just includes small kbuild test robot warnings caused by
ommited parameter names, and remove changelogs from patches description, moving
them to this cover-letter.


Changelog:

V7:

	- Rebased over linux-next
	- Add parameters names to function declarations
	  Reported-by: kbuild test robot <lkp@intel.com>
	- Remove changelog entries from patches's commit logs

V6:

        - Add a dummy bmap() definition so build does not break if
          CONFIG_BLOCK is not set
          Reported-by: kbuild test robot <lkp@intel.com>

        - ASSERT only if filesystem does not support bmap() to
          match the original logic

	- Fix bmap() doc function
	  Reported-by: kbuild test robot <lkp@intel.com>

V5:

        - Rebasing against 5.3 required changes to the f2fs
          check_swap_activate() function

V4:

        - Ensure ioctl_fibmap() returns 0 in case of error returned from
          bmap(). Otherwise we'll be changing the user interface (which
          returns 0 in case of error)
V3:
        - Rename usr_blk to ur_block

V2:
        - Use a local sector_t variable to asign the block number
          instead of using direct casting.



Carlos Maiolino (5):
  fs: Enable bmap() function to properly return errors
  cachefiles: drop direct usage of ->bmap method.
  ecryptfs: drop direct calls to ->bmap
  fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
  fibmap: Reject negative block numbers

 drivers/md/md-bitmap.c | 16 ++++++++++------
 fs/cachefiles/rdwr.c   | 27 ++++++++++++++-------------
 fs/ecryptfs/mmap.c     | 16 ++++++----------
 fs/f2fs/data.c         | 16 +++++++++++-----
 fs/inode.c             | 30 +++++++++++++++++-------------
 fs/ioctl.c             | 32 ++++++++++++++++++++++----------
 fs/jbd2/journal.c      | 22 +++++++++++++++-------
 include/linux/fs.h     |  9 ++++++++-
 mm/page_io.c           | 11 +++++++----
 9 files changed, 110 insertions(+), 69 deletions(-)

-- 
2.23.0

[PATCH 5/5] fibmap: Reject negative block numbers

[Carlos Maiolino]

FIBMAP receives an integer from userspace which is then implicitly converted
into sector_t to be passed to bmap(). No check is made to ensure userspace
didn't send a negative block number, which can end up in an underflow, and
returning to userspace a corrupted block address.

As a side-effect, the underflow caused by a negative block here, will
trigger the WARN() in iomap_bmap_actor(), which is how this issue was
first discovered.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 83f36feaf781..79468b5a6391 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -65,6 +65,9 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
 	if (error)
 		return error;
 
+	if (ur_block < 0)
+		return -EINVAL;
+
 	block = ur_block;
 	error = bmap(inode, &block);
 
-- 
2.23.0

[PATCH V8 0/5] Refactor ioctl_fibmap() internal interface

[Carlos Maiolino]

Hi,

This series refactor the internal structure of FIBMAP so that the filesystem can
properly report errors back to VFS, and also simplifies its usage by
standardizing all ->bmap() method usage via bmap() function.

The last patch is a bug fix for ioctl_fibmap() calls with negative block values.


Viro spotted a mistake in patch 4/5 on the previous version, where bmap()
return value was not being propagated back to userland, breaking its ABI.

So, this new version, only has a change on patch 4/5 to fix this problem.


Changelog:

V8:
	- Rebased over linux-next
	- Fix an error in patch 4/5, which led to the wrong value being
	  returned by ioctl_fibmap()
V7:

        - Rebased over linux-next
        - Add parameters names to function declarations
          Reported-by: kbuild test robot <lkp@intel.com>
        - Remove changelog entries from patches's commit logs

V6:

        - Add a dummy bmap() definition so build does not break if
          CONFIG_BLOCK is not set
          Reported-by: kbuild test robot <lkp@intel.com>

        - ASSERT only if filesystem does not support bmap() to
          match the original logic

        - Fix bmap() doc function
          Reported-by: kbuild test robot <lkp@intel.com>

V5:

        - Rebasing against 5.3 required changes to the f2fs
          check_swap_activate() function

V4:

        - Ensure ioctl_fibmap() returns 0 in case of error returned from
          bmap(). Otherwise we'll be changing the user interface (which
          returns 0 in case of error)
V3:
        - Rename usr_blk to ur_block

V2:
        - Use a local sector_t variable to asign the block number
          instead of using direct casting.


Carlos Maiolino (5):
  fs: Enable bmap() function to properly return errors
  cachefiles: drop direct usage of ->bmap method.
  ecryptfs: drop direct calls to ->bmap
  fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
  fibmap: Reject negative block numbers

 drivers/md/md-bitmap.c | 16 ++++++++++------
 fs/cachefiles/rdwr.c   | 27 ++++++++++++++-------------
 fs/ecryptfs/mmap.c     | 16 ++++++----------
 fs/f2fs/data.c         | 16 +++++++++++-----
 fs/inode.c             | 30 +++++++++++++++++-------------
 fs/ioctl.c             | 33 +++++++++++++++++++++++----------
 fs/jbd2/journal.c      | 22 +++++++++++++++-------
 include/linux/fs.h     |  9 ++++++++-
 mm/page_io.c           | 11 +++++++----
 9 files changed, 111 insertions(+), 69 deletions(-)

-- 
2.23.0

[PATCH 5/5] fibmap: Reject negative block numbers

[Carlos Maiolino]

FIBMAP receives an integer from userspace which is then implicitly converted
into sector_t to be passed to bmap(). No check is made to ensure userspace
didn't send a negative block number, which can end up in an underflow, and
returning to userspace a corrupted block address.

As a side-effect, the underflow caused by a negative block here, will
trigger the WARN() in iomap_bmap_actor(), which is how this issue was
first discovered.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 0ed5fb2d6c19..72d6848fb6ad 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -65,6 +65,9 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
 	if (error)
 		return error;
 
+	if (ur_block < 0)
+		return -EINVAL;
+
 	block = ur_block;
 	error = bmap(inode, &block);
 
-- 
2.23.0