[PATCH 0/2] btrfs: fix several sparse warnings

[PATCH 0/2] btrfs: fix several sparse warnings

[Omar Sandoval]

This patch series fixes some warnings reported by sparse when building the
btrfs module. It fixes two classes of warnings: address space warnings and lock
context warnings. This didn't uncover any logical errors, but it reduces the
noise when checking the source. There are a few less straightforward sparse
warnings still remaining.

Omar Sandoval (2):
  btrfs: fix sparse address space warnings
  btrfs: fix sparse lock context warnings

 fs/btrfs/ctree.c            | 3 ++-
 fs/btrfs/extent-tree.c      | 1 +
 fs/btrfs/extent_io.c        | 2 +-
 fs/btrfs/free-space-cache.c | 1 +
 fs/btrfs/ioctl.c            | 6 +++---
 fs/btrfs/locking.c          | 1 +
 fs/btrfs/send.c             | 8 ++++----
 7 files changed, 13 insertions(+), 9 deletions(-)

-- 
2.1.1

[PATCH 1/2] btrfs: fix sparse address space warnings

[Omar Sandoval]

The buffer passed to vfs_write in send and several casts of ioctl fields are
missing the __user annotation. Also fixes a couple of related trivial style
issues.

Signed-off-by: Omar Sandoval <osandov@osandov.com>
---
 fs/btrfs/ioctl.c | 6 +++---
 fs/btrfs/send.c  | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 8a8e298..0f9a5a1 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2176,7 +2176,7 @@ static noinline int btrfs_ioctl_tree_search_v2(struct file *file,
 
 	inode = file_inode(file);
 	ret = search_ioctl(inode, &args.key, &buf_size,
-			   (char *)(&uarg->buf[0]));
+			   (char __user *)(&uarg->buf[0]));
 	if (ret == 0 && copy_to_user(&uarg->key, &args.key, sizeof(args.key)))
 		ret = -EFAULT;
 	else if (ret == -EOVERFLOW &&
@@ -4247,7 +4247,7 @@ static long btrfs_ioctl_ino_to_path(struct btrfs_root *root, void __user *arg)
 		ipath->fspath->val[i] = rel_ptr;
 	}
 
-	ret = copy_to_user((void *)(unsigned long)ipa->fspath,
+	ret = copy_to_user((void __user *)(unsigned long)ipa->fspath,
 			   (void *)(unsigned long)ipath->fspath, size);
 	if (ret) {
 		ret = -EFAULT;
@@ -4322,7 +4322,7 @@ static long btrfs_ioctl_logical_to_ino(struct btrfs_root *root,
 	if (ret < 0)
 		goto out;
 
-	ret = copy_to_user((void *)(unsigned long)loi->inodes,
+	ret = copy_to_user((void __user *)(unsigned long)loi->inodes,
 			   (void *)(unsigned long)inodes, size);
 	if (ret)
 		ret = -EFAULT;
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 6528aa6..e0be577 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -515,7 +515,8 @@ static int write_buf(struct file *filp, const void *buf, u32 len, loff_t *off)
 	set_fs(KERNEL_DS);
 
 	while (pos < len) {
-		ret = vfs_write(filp, (char *)buf + pos, len - pos, off);
+		ret = vfs_write(filp, (__force const char __user *)buf + pos,
+				len - pos, off);
 		/* TODO handle that correctly */
 		/*if (ret == -ERESTARTSYS) {
 			continue;
@@ -639,8 +640,7 @@ static int send_header(struct send_ctx *sctx)
 	strcpy(hdr.magic, BTRFS_SEND_STREAM_MAGIC);
 	hdr.version = cpu_to_le32(BTRFS_SEND_STREAM_VERSION);
 
-	return write_buf(sctx->send_filp, &hdr, sizeof(hdr),
-					&sctx->send_off);
+	return write_buf(sctx->send_filp, &hdr, sizeof(hdr), &sctx->send_off);
 }
 
 /*
@@ -676,7 +676,7 @@ static int send_cmd(struct send_ctx *sctx)
 	hdr->crc = cpu_to_le32(crc);
 
 	ret = write_buf(sctx->send_filp, sctx->send_buf, sctx->send_size,
-					&sctx->send_off);
+			&sctx->send_off);
 
 	sctx->total_send_size += sctx->send_size;
 	sctx->cmd_send_size[le16_to_cpu(hdr->cmd)] += sctx->send_size;
-- 
2.1.1

[PATCH 2/2] btrfs: fix sparse lock context warnings

[Omar Sandoval]

Fix several sparse warnings that can easily be addressed with context
annotations. These annotations also provide some sort of documentation for the
internal helper functions.

Signed-off-by: Omar Sandoval <osandov@osandov.com>
---
 fs/btrfs/ctree.c            | 3 ++-
 fs/btrfs/extent-tree.c      | 1 +
 fs/btrfs/extent_io.c        | 2 +-
 fs/btrfs/free-space-cache.c | 1 +
 fs/btrfs/locking.c          | 1 +
 5 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 44ee5d2..c1bbc88 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -487,7 +487,8 @@ __tree_mod_log_insert(struct btrfs_fs_info *fs_info, struct tree_mod_elem *tm)
  * call tree_mod_log_write_unlock() to release.
  */
 static inline int tree_mod_dont_log(struct btrfs_fs_info *fs_info,
-				    struct extent_buffer *eb) {
+				    struct extent_buffer *eb)
+{
 	smp_mb();
 	if (list_empty(&(fs_info)->tree_mod_seq_list))
 		return 1;
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 3efe1c3..281f30f 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -6387,6 +6387,7 @@ static struct btrfs_block_group_cache *
 btrfs_lock_cluster(struct btrfs_block_group_cache *block_group,
 		   struct btrfs_free_cluster *cluster,
 		   int delalloc)
+__acquires(&cluster->refill_lock)
 {
 	struct btrfs_block_group_cache *used_bg;
 	bool locked = false;
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index af0359d..cc6b1fc 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4775,8 +4775,8 @@ static inline void btrfs_release_extent_buffer_rcu(struct rcu_head *head)
 	__free_extent_buffer(eb);
 }
 
-/* Expects to have eb->eb_lock already held */
 static int release_extent_buffer(struct extent_buffer *eb)
+__releases(&eb->refs_lock)
 {
 	WARN_ON(atomic_read(&eb->refs) == 0);
 	if (atomic_dec_and_test(&eb->refs)) {
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index 2b0a627..41c6cd5 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -1838,6 +1838,7 @@ static struct btrfs_free_space_op free_space_op = {
 
 static int insert_into_bitmap(struct btrfs_free_space_ctl *ctl,
 			      struct btrfs_free_space *info)
+__must_hold(&ctl->tree_lock)
 {
 	struct btrfs_free_space *bitmap_info;
 	struct btrfs_block_group_cache *block_group = NULL;
diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index 5665d21..47bdc2c 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -222,6 +222,7 @@ void btrfs_tree_read_unlock_blocking(struct extent_buffer *eb)
  * blocking readers or writers
  */
 void btrfs_tree_lock(struct extent_buffer *eb)
+__acquires(&eb->lock)
 {
 again:
 	wait_event(eb->read_lock_wq, atomic_read(&eb->blocking_readers) == 0);
-- 
2.1.1

Re: [PATCH 0/2] btrfs: fix several sparse warnings

[Omar Sandoval]

On Sun, Sep 28, 2014 at 01:48:10AM -0700, Omar Sandoval wrote:
> This patch series fixes some warnings reported by sparse when building the
> btrfs module. It fixes two classes of warnings: address space warnings and lock
> context warnings. This didn't uncover any logical errors, but it reduces the
> noise when checking the source. There are a few less straightforward sparse
> warnings still remaining.
> 
> Omar Sandoval (2):
>   btrfs: fix sparse address space warnings
>   btrfs: fix sparse lock context warnings
> 
>  fs/btrfs/ctree.c            | 3 ++-
>  fs/btrfs/extent-tree.c      | 1 +
>  fs/btrfs/extent_io.c        | 2 +-
>  fs/btrfs/free-space-cache.c | 1 +
>  fs/btrfs/ioctl.c            | 6 +++---
>  fs/btrfs/locking.c          | 1 +
>  fs/btrfs/send.c             | 8 ++++----
>  7 files changed, 13 insertions(+), 9 deletions(-)
> 

This patch series applies to 3.17-rc6.

-- 
Omar

Re: [PATCH 1/2] btrfs: fix sparse address space warnings

[Omar Sandoval]

On Sun, Sep 28, 2014 at 01:48:11AM -0700, Omar Sandoval wrote:
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index 6528aa6..e0be577 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -515,7 +515,8 @@ static int write_buf(struct file *filp, const void *buf, u32 len, loff_t *off)
>  	set_fs(KERNEL_DS);
>  
>  	while (pos < len) {
> -		ret = vfs_write(filp, (char *)buf + pos, len - pos, off);
> +		ret = vfs_write(filp, (__force const char __user *)buf + pos,
> +				len - pos, off);
>  		/* TODO handle that correctly */
>  		/*if (ret == -ERESTARTSYS) {
>  			continue;

Actually, looking at this now, it looks like this is just an open-coded
kernel_write. I think this could be made a bit cleaner by using that instead;
the tradeoff is that each call to kernel_write will do the address space
flip-flop, so if the write gets split up into many calls, there'd be some
slight overhead. That's probably a microoptimization, but I think it's worth
looking into making kernel_read and kernel_write handle the retry logic.

It looks like Oren Laadan submitted a patch doing exactly that as part of the
checkpoint/restart patch series: https://lkml.org/lkml/2010/5/6/142. I've added
an email address that I found for him.

>From Josef's response way back in 2010:
> > +static ssize_t _kernel_write(struct file *file, loff_t offset,
> > +			     const char __user *ubuf, size_t count)
> > +{
> > +	ssize_t nwrite;
> > +	size_t nleft;
> > +	loff_t pos = offset;
> > +
> > +	for (nleft = count; nleft; nleft -= nwrite) {
> > +		nwrite = vfs_write(file, ubuf, nleft, &pos);
> > +		if (nwrite < 0) {
> > +			if (nwrite == -EAGAIN) {
> > +				nwrite = 0;
> > +				continue;
> > +			} else
> > +				return nwrite;
> > +		}
> > +		ubuf += nwrite;
> > +	}
> > +	return count - nleft;
> > +}
> 
> I'm not entirely sure if this can happen, but if vfs_write doesn't write
> anything, but doesn't have an error, we could end up in an infinite loop.  Like
> I said I'm not sure if thats even possible, but its definitely one of those
> things that if it is possible some random security guy is going to figure out
> how to exploit it at some point down the line.

Did anyone ever come up with a good answer for this?

-- 
Omar

Re: [PATCH 1/2] btrfs: fix sparse address space warnings

[David Sterba]

On Sun, Sep 28, 2014 at 01:48:11AM -0700, Omar Sandoval wrote:
> The buffer passed to vfs_write in send and several casts of ioctl fields are
> missing the __user annotation. Also fixes a couple of related trivial style
> issues.
> 
> Signed-off-by: Omar Sandoval <osandov@osandov.com>

Reviewed-by: David Sterba <dsterba@suse.cz>

> @@ -639,8 +640,7 @@ static int send_header(struct send_ctx *sctx)
> -	return write_buf(sctx->send_filp, &hdr, sizeof(hdr),
> -					&sctx->send_off);
> +	return write_buf(sctx->send_filp, &hdr, sizeof(hdr), &sctx->send_off);

>  	ret = write_buf(sctx->send_filp, sctx->send_buf, sctx->send_size,
> -					&sctx->send_off);
> +			&sctx->send_off);

Please do not fold unrelated changes.

Re: [PATCH 2/2] btrfs: fix sparse lock context warnings

[David Sterba]

On Sun, Sep 28, 2014 at 01:48:12AM -0700, Omar Sandoval wrote:
> Fix several sparse warnings that can easily be addressed with context
> annotations. These annotations also provide some sort of documentation for the
> internal helper functions.
> 
> Signed-off-by: Omar Sandoval <osandov@osandov.com>

Reviewed-by: David Sterba <dsterba@suse.cz>

Re: [PATCH 1/2] btrfs: fix sparse address space warnings

[Omar Sandoval]

On Mon, Sep 29, 2014 at 06:07:38PM +0200, David Sterba wrote:
> On Sun, Sep 28, 2014 at 01:48:11AM -0700, Omar Sandoval wrote:
> > The buffer passed to vfs_write in send and several casts of ioctl fields are
> > missing the __user annotation. Also fixes a couple of related trivial style
> > issues.
> > 
> > Signed-off-by: Omar Sandoval <osandov@osandov.com>
> 
> Reviewed-by: David Sterba <dsterba@suse.cz>

Thank you.

> > @@ -639,8 +640,7 @@ static int send_header(struct send_ctx *sctx)
> > -	return write_buf(sctx->send_filp, &hdr, sizeof(hdr),
> > -					&sctx->send_off);
> > +	return write_buf(sctx->send_filp, &hdr, sizeof(hdr), &sctx->send_off);
> 
> >  	ret = write_buf(sctx->send_filp, sctx->send_buf, sctx->send_size,
> > -					&sctx->send_off);
> > +			&sctx->send_off);
> 
> Please do not fold unrelated changes.

My metric for "related" here was that these were call sites of a function I
directly modified. Is the preferred form to just split style fixes that we
encounter into a separate patch in the series?

-- 
Omar

Re: [PATCH 1/2] btrfs: fix sparse address space warnings

[David Sterba]

On Mon, Sep 29, 2014 at 12:45:12PM -0700, Omar Sandoval wrote:
> > > @@ -639,8 +640,7 @@ static int send_header(struct send_ctx *sctx)
> > > -	return write_buf(sctx->send_filp, &hdr, sizeof(hdr),
> > > -					&sctx->send_off);
> > > +	return write_buf(sctx->send_filp, &hdr, sizeof(hdr), &sctx->send_off);
> > 
> > >  	ret = write_buf(sctx->send_filp, sctx->send_buf, sctx->send_size,
> > > -					&sctx->send_off);
> > > +			&sctx->send_off);
> > 
> > Please do not fold unrelated changes.
> 
> My metric for "related" here was that these were call sites of a function I
> directly modified.

The changes are only in the whitespace, that's not necessary. It's
usually ok to fix style issues in the code you modify directly.

> Is the preferred form to just split style fixes that we encounter into
> a separate patch in the series?

Well, I may only express my point of view. Yes, split the style-only
changes into another patch and don't send it :)

The problem with patches that do not effectively change anything is that
they pollute git history and just add extra step when one has to look
for a patch that broke something, or eg. change context of following
patches and make backporting a bit more tedious. Code cleanups are fine,
but there's usually a point of making the code more readable, compact,
etc.

The coding style should be perfect from the beginning. Nobody will
probably point out minor style violations during review, because it just
pointless for a patch that fixes a real bug.

Re: [PATCH 1/2] btrfs: fix sparse address space warnings

[Zach Brown]

On Sun, Sep 28, 2014 at 03:26:04PM -0700, Omar Sandoval wrote:
> On Sun, Sep 28, 2014 at 01:48:11AM -0700, Omar Sandoval wrote:
> > diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> > index 6528aa6..e0be577 100644
> > --- a/fs/btrfs/send.c
> > +++ b/fs/btrfs/send.c
> > @@ -515,7 +515,8 @@ static int write_buf(struct file *filp, const void *buf, u32 len, loff_t *off)
> >  	set_fs(KERNEL_DS);
> >  
> >  	while (pos < len) {
> > -		ret = vfs_write(filp, (char *)buf + pos, len - pos, off);
> > +		ret = vfs_write(filp, (__force const char __user *)buf + pos,
> > +				len - pos, off);
> >  		/* TODO handle that correctly */
> >  		/*if (ret == -ERESTARTSYS) {
> >  			continue;
> 
> Actually, looking at this now, it looks like this is just an open-coded
> kernel_write. I think this could be made a bit cleaner by using that instead;

Agreed, but notice that you'll want to be careful to update
write_buf()'s *off because passing a dereferenced copy to kernel_write()
will lose the pos update that vfs_write() is currently taking care of.

A carefully placed "*off += ret" in write_buf() will be fine.  (As fine
as having a magical private file position in the send context ever was.)

> the tradeoff is that each call to kernel_write will do the address
> space flip-flop, so if the write gets split up into many calls,
> there'd be some slight overhead.  That's probably a microoptimization,
> but 

Yeah, I don't think that overhead is going to be significant given all
of the work that's going on.

> I think it's worth looking
> into making kernel_read and kernel_write handle the retry logic.

I disagree.  I wouldn't broaden the scope to add retrying on behalf of
all kernel_write() callers and write methods (it's exported to modules,
too).  I'd leave the looping in btrfs and just call kernel_write() to
get rid of the segment juggling.

- z

Re: [PATCH 1/2] btrfs: fix sparse address space warnings

[Omar Sandoval]

On Mon, Sep 29, 2014 at 11:49:54PM +0200, David Sterba wrote:
> On Mon, Sep 29, 2014 at 12:45:12PM -0700, Omar Sandoval wrote:
> > > > @@ -639,8 +640,7 @@ static int send_header(struct send_ctx *sctx)
> > > > -	return write_buf(sctx->send_filp, &hdr, sizeof(hdr),
> > > > -					&sctx->send_off);
> > > > +	return write_buf(sctx->send_filp, &hdr, sizeof(hdr), &sctx->send_off);
> > > 
> > > >  	ret = write_buf(sctx->send_filp, sctx->send_buf, sctx->send_size,
> > > > -					&sctx->send_off);
> > > > +			&sctx->send_off);
> > > 
> > > Please do not fold unrelated changes.
> > 
> > My metric for "related" here was that these were call sites of a function I
> > directly modified.
> 
> The changes are only in the whitespace, that's not necessary. It's
> usually ok to fix style issues in the code you modify directly.
> 
> > Is the preferred form to just split style fixes that we encounter into
> > a separate patch in the series?
> 
> Well, I may only express my point of view. Yes, split the style-only
> changes into another patch and don't send it :)
> 
> The problem with patches that do not effectively change anything is that
> they pollute git history and just add extra step when one has to look
> for a patch that broke something, or eg. change context of following
> patches and make backporting a bit more tedious. Code cleanups are fine,
> but there's usually a point of making the code more readable, compact,
> etc.
> 
> The coding style should be perfect from the beginning. Nobody will
> probably point out minor style violations during review, because it just
> pointless for a patch that fixes a real bug.

Thank you for the perspective :)


On Tue, Sep 30, 2014 at 12:27:43PM -0700, Zach Brown wrote:
> On Sun, Sep 28, 2014 at 03:26:04PM -0700, Omar Sandoval wrote:
> > On Sun, Sep 28, 2014 at 01:48:11AM -0700, Omar Sandoval wrote:
> > > diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> > > index 6528aa6..e0be577 100644
> > > --- a/fs/btrfs/send.c
> > > +++ b/fs/btrfs/send.c
> > > @@ -515,7 +515,8 @@ static int write_buf(struct file *filp, const void *buf, u32 len, loff_t *off)
> > >  	set_fs(KERNEL_DS);
> > >  
> > >  	while (pos < len) {
> > > -		ret = vfs_write(filp, (char *)buf + pos, len - pos, off);
> > > +		ret = vfs_write(filp, (__force const char __user *)buf + pos,
> > > +				len - pos, off);
> > >  		/* TODO handle that correctly */
> > >  		/*if (ret == -ERESTARTSYS) {
> > >  			continue;
> > 
> > Actually, looking at this now, it looks like this is just an open-coded
> > kernel_write. I think this could be made a bit cleaner by using that instead;
> 
> Agreed, but notice that you'll want to be careful to update
> write_buf()'s *off because passing a dereferenced copy to kernel_write()
> will lose the pos update that vfs_write() is currently taking care of.
> 
> A carefully placed "*off += ret" in write_buf() will be fine.  (As fine
> as having a magical private file position in the send context ever was.)
> 
> > the tradeoff is that each call to kernel_write will do the address
> > space flip-flop, so if the write gets split up into many calls,
> > there'd be some slight overhead.  That's probably a microoptimization,
> > but 
> 
> Yeah, I don't think that overhead is going to be significant given all
> of the work that's going on.
> 
> > I think it's worth looking
> > into making kernel_read and kernel_write handle the retry logic.
> 
> I disagree.  I wouldn't broaden the scope to add retrying on behalf of
> all kernel_write() callers and write methods (it's exported to modules,
> too).  I'd leave the looping in btrfs and just call kernel_write() to
> get rid of the segment juggling.
> 
> - z

That sounds fair. I'll submit a v2 that replaces vfs_write with kernel_write.

-- 
Omar