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

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: <20140930192743.GE12900@lenny.home.zabbo.net>
Date:	Tue, 30 Sep 2014 12:27:43 -0700
From:	Zach Brown <zab@...bo.net>
To:	Omar Sandoval <osandov@...ndov.com>
Cc:	Chris Mason <clm@...com>, Josef Bacik <jbacik@...com>,
	linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org,
	orenl@...lrox.com
Subject: Re: [PATCH 1/2] btrfs: fix sparse address space warnings

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
--
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/