| Message ID | 1439640464-785-1-git-send-email-vincent@bernat.im (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Eric Van Hensbergen |
| Headers | show |
Vincent Bernat wrote on Sat, Aug 15, 2015: > When reading 0 bytes from an empty file on a 9P filesystem, the return > code of read() was not 0 as expected due to an unitialized err variable. Good find! > diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c > index 1ef16bd8280b..3abc447783aa 100644 > --- a/fs/9p/vfs_file.c > +++ b/fs/9p/vfs_file.c > @@ -381,7 +381,7 @@ static ssize_t > v9fs_file_read_iter(struct kiocb *iocb, struct iov_iter *to) > { > struct p9_fid *fid = iocb->ki_filp->private_data; > - int ret, err; > + int ret, err = 0; > > p9_debug(P9_DEBUG_VFS, "count %zu offset %lld\n", > iov_iter_count(to), iocb->ki_pos); There are plenty of other functions calling p9_client_read/write with the same issue e.g. for reads: v9fs_fid_readpage, v9fs_fid_xattr_get and write: v9fs_file_write_iter, v9fs_vfs_writepage_locked (there are other calls that look safe to me) I don't know if 0-length write makes sense, I think it's OK reading the manpage, but it can't hurt initializing anyway. Some e.g. v9fs_fid_readpage actually only check err and not the return value, but err is left untouched if no error now! Could you send a patch with all the fixes at once, or shall I look at it? Thanks,
? 15 août 2015 15:03 +0200, Dominique Martinet <asmadeus@codewreck.org> : >> diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c >> index 1ef16bd8280b..3abc447783aa 100644 >> --- a/fs/9p/vfs_file.c >> +++ b/fs/9p/vfs_file.c >> @@ -381,7 +381,7 @@ static ssize_t >> v9fs_file_read_iter(struct kiocb *iocb, struct iov_iter *to) >> { >> struct p9_fid *fid = iocb->ki_filp->private_data; >> - int ret, err; >> + int ret, err = 0; >> >> p9_debug(P9_DEBUG_VFS, "count %zu offset %lld\n", >> iov_iter_count(to), iocb->ki_pos); > > There are plenty of other functions calling p9_client_read/write with > the same issue e.g. for reads: > v9fs_fid_readpage, v9fs_fid_xattr_get > and write: > v9fs_file_write_iter, v9fs_vfs_writepage_locked > > (there are other calls that look safe to me) > > I don't know if 0-length write makes sense, I think it's OK reading the > manpage, but it can't hurt initializing anyway. > Some e.g. v9fs_fid_readpage actually only check err and not the return > value, but err is left untouched if no error now! > > > Could you send a patch with all the fixes at once, or shall I look at > it? Yes, I can look at all those cases and tuck everything in the same commit.
? 15 août 2015 15:03 +0200, Dominique Martinet <asmadeus@codewreck.org> : > There are plenty of other functions calling p9_client_read/write with > the same issue e.g. for reads: > v9fs_fid_readpage, v9fs_fid_xattr_get > and write: > v9fs_file_write_iter, v9fs_vfs_writepage_locked > > (there are other calls that look safe to me) > > I don't know if 0-length write makes sense, I think it's OK reading the > manpage, but it can't hurt initializing anyway. > Some e.g. v9fs_fid_readpage actually only check err and not the return > value, but err is left untouched if no error now! > > > Could you send a patch with all the fixes at once, or shall I look at > it? What about fixing in p9_client_read/write instead? It would be more future proof. I don't think that either of those will have a policy of not touching err if count is 0 (that would make little sense). So, any call should expect err to be altered. A patch will follow shortly for this proposition.
Vincent Bernat wrote on Sat, Aug 15, 2015: > What about fixing in p9_client_read/write instead? It would be more > future proof. I don't think that either of those will have a policy of > not touching err if count is 0 (that would make little sense). So, any > call should expect err to be altered. A patch will follow shortly for > this proposition. I think it really is a caller's logic problem but I have no idea if there are kernel style guidelines for that... A quick grep for '*err' shows quite a few functions, some unconditionally set a safe value while others don't - I guess that means we can do whichever. Happy either way myself, if you think it's cleaner to set it in p9_client_read/write themselves go for it. I'm just another monkey :)
diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c index 1ef16bd8280b..3abc447783aa 100644 --- a/fs/9p/vfs_file.c +++ b/fs/9p/vfs_file.c @@ -381,7 +381,7 @@ static ssize_t v9fs_file_read_iter(struct kiocb *iocb, struct iov_iter *to) { struct p9_fid *fid = iocb->ki_filp->private_data; - int ret, err; + int ret, err = 0; p9_debug(P9_DEBUG_VFS, "count %zu offset %lld\n", iov_iter_count(to), iocb->ki_pos);
When reading 0 bytes from an empty file on a 9P filesystem, the return code of read() was not 0 as expected due to an unitialized err variable. Tested with this simple program: #include <assert.h> #include <sys/types.h> #include <sys/stat.h> #include <fcntl.h> #include <unistd.h> int main(int argc, const char **argv) { assert(argc == 2); char buffer[256]; int fd = open(argv[1], O_RDONLY|O_NOCTTY); assert(fd >= 0); assert(read(fd, buffer, 0) == 0); return 0; } Signed-off-by: Vincent Bernat <vincent@bernat.im> --- fs/9p/vfs_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)