| Message ID | 1411894092-28784-2-git-send-email-osandov@osandov.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)