| Message ID | 20240103145935.384404-2-dhowells@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | netfs, cachefiles, 9p: Additional patches | expand |
On Wed, Jan 03, 2024 at 02:59:25PM +0000, David Howells wrote: > Fix __cachefiles_prepare_write() to correctly determine whether the > requested write will fit correctly with the DIO alignment. > > Reported-by: Gao Xiang <hsiangkao@linux.alibaba.com> > Signed-off-by: David Howells <dhowells@redhat.com> > Tested-by: Yiqun Leng <yqleng@linux.alibaba.com> > Tested-by: Jia Zhu <zhujia.zj@bytedance.com> > cc: Jeff Layton <jlayton@kernel.org> > cc: linux-cachefs@redhat.com > cc: linux-erofs@lists.ozlabs.org > cc: linux-fsdevel@vger.kernel.org > cc: linux-mm@kvack.org > --- > fs/cachefiles/io.c | 28 +++++++++++++++++----------- > 1 file changed, 17 insertions(+), 11 deletions(-) > > diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c > index bffffedce4a9..7529b40bc95a 100644 > --- a/fs/cachefiles/io.c > +++ b/fs/cachefiles/io.c > @@ -522,16 +522,22 @@ int __cachefiles_prepare_write(struct cachefiles_object *object, > bool no_space_allocated_yet) > { > struct cachefiles_cache *cache = object->volume->cache; > - loff_t start = *_start, pos; > - size_t len = *_len, down; > + unsigned long long start = *_start, pos; > + size_t len = *_len; > int ret; > > /* Round to DIO size */ > - down = start - round_down(start, PAGE_SIZE); > - *_start = start - down; > - *_len = round_up(down + len, PAGE_SIZE); > - if (down < start || *_len > upper_len) > + start = round_down(*_start, PAGE_SIZE); > + if (start != *_start) { > + kleave(" = -ENOBUFS [down]"); > + return -ENOBUFS; > + } > + if (*_len > upper_len) { > + kleave(" = -ENOBUFS [up]"); > return -ENOBUFS; > + } > + > + *_len = round_up(len, PAGE_SIZE); > > /* We need to work out whether there's sufficient disk space to perform > * the write - but we can skip that check if we have space already > @@ -542,7 +548,7 @@ int __cachefiles_prepare_write(struct cachefiles_object *object, > > pos = cachefiles_inject_read_error(); > if (pos == 0) > - pos = vfs_llseek(file, *_start, SEEK_DATA); > + pos = vfs_llseek(file, start, SEEK_DATA); > if (pos < 0 && pos >= (loff_t)-MAX_ERRNO) { Hi David, I realise these patches have been accepted, but I have a minor nit: pos is now unsigned, and so cannot be less than zero. Flagged by Smatch and Coccinelle. > if (pos == -ENXIO) > goto check_space; /* Unallocated tail */ > @@ -550,7 +556,7 @@ int __cachefiles_prepare_write(struct cachefiles_object *object, > cachefiles_trace_seek_error); > return pos; > } > - if ((u64)pos >= (u64)*_start + *_len) > + if (pos >= start + *_len) > goto check_space; /* Unallocated region */ > > /* We have a block that's at least partially filled - if we're low on > @@ -563,13 +569,13 @@ int __cachefiles_prepare_write(struct cachefiles_object *object, > > pos = cachefiles_inject_read_error(); > if (pos == 0) > - pos = vfs_llseek(file, *_start, SEEK_HOLE); > + pos = vfs_llseek(file, start, SEEK_HOLE); > if (pos < 0 && pos >= (loff_t)-MAX_ERRNO) { Ditto. > trace_cachefiles_io_error(object, file_inode(file), pos, > cachefiles_trace_seek_error); > return pos; > } > - if ((u64)pos >= (u64)*_start + *_len) > + if (pos >= start + *_len) > return 0; /* Fully allocated */ > > /* Partially allocated, but insufficient space: cull. */ > @@ -577,7 +583,7 @@ int __cachefiles_prepare_write(struct cachefiles_object *object, > ret = cachefiles_inject_remove_error(); > if (ret == 0) > ret = vfs_fallocate(file, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, > - *_start, *_len); > + start, *_len); > if (ret < 0) { > trace_cachefiles_io_error(object, file_inode(file), ret, > cachefiles_trace_fallocate_error); >
Simon Horman <horms@kernel.org> wrote: > I realise these patches have been accepted, but I have a minor nit: > pos is now unsigned, and so cannot be less than zero. Good point. How about the attached patch. Whilst I would prefer to use unsigned long long to avoid the casts, it might David --- cachefiles: Fix signed/unsigned mixup In __cachefiles_prepare_write(), the start and pos variables were made unsigned 64-bit so that the casts in the checking could be got rid of - which should be fine since absolute file offsets can't be negative, except that an error code may be obtained from vfs_llseek(), which *would* be negative. This breaks the error check. Fix this for now by reverting pos and start to be signed and putting back the casts. Unfortunately, the error value checks cannot be replaced with IS_ERR_VALUE() as long might be 32-bits. Fixes: 7097c96411d2 ("cachefiles: Fix __cachefiles_prepare_write()") Reported-by: Simon Horman <horms@kernel.org> Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202401071152.DbKqMQMu-lkp@intel.com/ Signed-off-by: David Howells <dhowells@redhat.com> cc: Gao Xiang <hsiangkao@linux.alibaba.com> cc: Yiqun Leng <yqleng@linux.alibaba.com> cc: Jia Zhu <zhujia.zj@bytedance.com> cc: Jeff Layton <jlayton@kernel.org> cc: linux-cachefs@redhat.com cc: linux-erofs@lists.ozlabs.org cc: linux-fsdevel@vger.kernel.org cc: linux-mm@kvack.org --- fs/cachefiles/io.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c index 3eec26967437..9a2cb2868e90 100644 --- a/fs/cachefiles/io.c +++ b/fs/cachefiles/io.c @@ -522,7 +522,7 @@ int __cachefiles_prepare_write(struct cachefiles_object *object, bool no_space_allocated_yet) { struct cachefiles_cache *cache = object->volume->cache; - unsigned long long start = *_start, pos; + loff_t start = *_start, pos; size_t len = *_len; int ret; @@ -556,7 +556,7 @@ int __cachefiles_prepare_write(struct cachefiles_object *object, cachefiles_trace_seek_error); return pos; } - if (pos >= start + *_len) + if ((u64)pos >= (u64)start + *_len) goto check_space; /* Unallocated region */ /* We have a block that's at least partially filled - if we're low on @@ -575,7 +575,7 @@ int __cachefiles_prepare_write(struct cachefiles_object *object, cachefiles_trace_seek_error); return pos; } - if (pos >= start + *_len) + if ((u64)pos >= (u64)start + *_len) return 0; /* Fully allocated */ /* Partially allocated, but insufficient space: cull. */
On Mon, Jan 08, 2024 at 10:31:30PM +0000, David Howells wrote: > Simon Horman <horms@kernel.org> wrote: > > > I realise these patches have been accepted, but I have a minor nit: > > pos is now unsigned, and so cannot be less than zero. > > Good point. How about the attached patch. Whilst I would prefer to use > unsigned long long to avoid the casts, it might Hi David, I would also prefer to avoid casts, but I agree this is a good way forward. Thanks for the quick fix. Reviewed-by: Simon Horman <horms@kernel.org>
diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c index bffffedce4a9..7529b40bc95a 100644 --- a/fs/cachefiles/io.c +++ b/fs/cachefiles/io.c @@ -522,16 +522,22 @@ int __cachefiles_prepare_write(struct cachefiles_object *object, bool no_space_allocated_yet) { struct cachefiles_cache *cache = object->volume->cache; - loff_t start = *_start, pos; - size_t len = *_len, down; + unsigned long long start = *_start, pos; + size_t len = *_len; int ret; /* Round to DIO size */ - down = start - round_down(start, PAGE_SIZE); - *_start = start - down; - *_len = round_up(down + len, PAGE_SIZE); - if (down < start || *_len > upper_len) + start = round_down(*_start, PAGE_SIZE); + if (start != *_start) { + kleave(" = -ENOBUFS [down]"); + return -ENOBUFS; + } + if (*_len > upper_len) { + kleave(" = -ENOBUFS [up]"); return -ENOBUFS; + } + + *_len = round_up(len, PAGE_SIZE); /* We need to work out whether there's sufficient disk space to perform * the write - but we can skip that check if we have space already @@ -542,7 +548,7 @@ int __cachefiles_prepare_write(struct cachefiles_object *object, pos = cachefiles_inject_read_error(); if (pos == 0) - pos = vfs_llseek(file, *_start, SEEK_DATA); + pos = vfs_llseek(file, start, SEEK_DATA); if (pos < 0 && pos >= (loff_t)-MAX_ERRNO) { if (pos == -ENXIO) goto check_space; /* Unallocated tail */ @@ -550,7 +556,7 @@ int __cachefiles_prepare_write(struct cachefiles_object *object, cachefiles_trace_seek_error); return pos; } - if ((u64)pos >= (u64)*_start + *_len) + if (pos >= start + *_len) goto check_space; /* Unallocated region */ /* We have a block that's at least partially filled - if we're low on @@ -563,13 +569,13 @@ int __cachefiles_prepare_write(struct cachefiles_object *object, pos = cachefiles_inject_read_error(); if (pos == 0) - pos = vfs_llseek(file, *_start, SEEK_HOLE); + pos = vfs_llseek(file, start, SEEK_HOLE); if (pos < 0 && pos >= (loff_t)-MAX_ERRNO) { trace_cachefiles_io_error(object, file_inode(file), pos, cachefiles_trace_seek_error); return pos; } - if ((u64)pos >= (u64)*_start + *_len) + if (pos >= start + *_len) return 0; /* Fully allocated */ /* Partially allocated, but insufficient space: cull. */ @@ -577,7 +583,7 @@ int __cachefiles_prepare_write(struct cachefiles_object *object, ret = cachefiles_inject_remove_error(); if (ret == 0) ret = vfs_fallocate(file, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, - *_start, *_len); + start, *_len); if (ret < 0) { trace_cachefiles_io_error(object, file_inode(file), ret, cachefiles_trace_fallocate_error);