| Message ID | 2d5fa5e3-dac5-6973-74e5-eeedf36a42b@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [next] shmem: minor fixes to splice-read implementation | expand |
Hugh Dickins <hughd@google.com> wrote: > + if (unlikely(!len)) > + return 0; Should this be done in do_splice_to() instead? Also, is it too late for Jens to fold this into the original patch since it's not upstream yet? Otherwise: Reviewed-by: David Howells <dhowells@redhat.com>
On 17.04.23 06:46, Hugh Dickins wrote: > generic_file_splice_read() makes a couple of preliminary checks (for > s_maxbytes and zero len), but shmem_file_splice_read() is called without > those: so check them inside it. (But shmem does not support O_DIRECT, > so no need for that one here - and even if O_DIRECT support were stubbed > in, it would still just be using the page cache.) > > HWPoison: my reading of folio_test_hwpoison() is that it only tests the > head page of a large folio, whereas splice_folio_into_pipe() will splice > as much of the folio as it can: so for safety we should also check the > has_hwpoisoned flag, set if any of the folio's pages are hwpoisoned. > (Perhaps that ugliness can be improved at the mm end later.) > > The call to splice_zeropage_into_pipe() risked overrunning past EOF: > ask it for "part" not "len". > > Fixes: b81d7b89becc ("shmem: Implement splice-read") > Signed-off-by: Hugh Dickins <hughd@google.com> > --- > Thank you, David, for attending to tmpfs in your splice update: > yes, I too wish it could have just used the generic, but I'm sure > you're right that there's a number of reasons it needs its own. > > mm/shmem.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -2902,6 +2902,11 @@ static ssize_t shmem_file_splice_read(struct file *in, loff_t *ppos, > loff_t isize; > int error = 0; > > + if (unlikely(*ppos >= MAX_LFS_FILESIZE)) > + return 0; > + if (unlikely(!len)) > + return 0; > + > /* Work out how much data we can actually add into the pipe */ > used = pipe_occupancy(pipe->head, pipe->tail); > npages = max_t(ssize_t, pipe->max_usage - used, 0); > @@ -2911,7 +2916,8 @@ static ssize_t shmem_file_splice_read(struct file *in, loff_t *ppos, > if (*ppos >= i_size_read(inode)) > break; > > - error = shmem_get_folio(inode, *ppos / PAGE_SIZE, &folio, SGP_READ); > + error = shmem_get_folio(inode, *ppos / PAGE_SIZE, &folio, > + SGP_READ); > if (error) { > if (error == -EINVAL) > error = 0; > @@ -2920,7 +2926,9 @@ static ssize_t shmem_file_splice_read(struct file *in, loff_t *ppos, > if (folio) { > folio_unlock(folio); > > - if (folio_test_hwpoison(folio)) { > + if (folio_test_hwpoison(folio) || > + (folio_test_large(folio) && > + folio_test_has_hwpoisoned(folio))) { > error = -EIO; > break; > } > @@ -2956,7 +2964,7 @@ static ssize_t shmem_file_splice_read(struct file *in, loff_t *ppos, > folio_put(folio); > folio = NULL; > } else { > - n = splice_zeropage_into_pipe(pipe, *ppos, len); > + n = splice_zeropage_into_pipe(pipe, *ppos, part); > } > > if (!n) > FWIW, looks good to me.
On Sun, 16 Apr 2023 21:46:16 -0700, Hugh Dickins wrote: > generic_file_splice_read() makes a couple of preliminary checks (for > s_maxbytes and zero len), but shmem_file_splice_read() is called without > those: so check them inside it. (But shmem does not support O_DIRECT, > so no need for that one here - and even if O_DIRECT support were stubbed > in, it would still just be using the page cache.) > > HWPoison: my reading of folio_test_hwpoison() is that it only tests the > head page of a large folio, whereas splice_folio_into_pipe() will splice > as much of the folio as it can: so for safety we should also check the > has_hwpoisoned flag, set if any of the folio's pages are hwpoisoned. > (Perhaps that ugliness can be improved at the mm end later.) > > [...] Applied, thanks! [1/1] shmem: minor fixes to splice-read implementation commit: 72887c976a7c9ee7527f4a2e3d109576efea98ab Best regards,
On Mon, 17 Apr 2023, Jens Axboe wrote: > On Sun, 16 Apr 2023 21:46:16 -0700, Hugh Dickins wrote: > > generic_file_splice_read() makes a couple of preliminary checks (for > > s_maxbytes and zero len), but shmem_file_splice_read() is called without > > those: so check them inside it. (But shmem does not support O_DIRECT, > > so no need for that one here - and even if O_DIRECT support were stubbed > > in, it would still just be using the page cache.) > > > > HWPoison: my reading of folio_test_hwpoison() is that it only tests the > > head page of a large folio, whereas splice_folio_into_pipe() will splice > > as much of the folio as it can: so for safety we should also check the > > has_hwpoisoned flag, set if any of the folio's pages are hwpoisoned. > > (Perhaps that ugliness can be improved at the mm end later.) > > > > [...] > > Applied, thanks! > > [1/1] shmem: minor fixes to splice-read implementation > commit: 72887c976a7c9ee7527f4a2e3d109576efea98ab > > Best regards, > -- > Jens Axboe Thanks, but it then vanished amidst the subsequent splice convulsions, and I can't quite tell how much of it is still needed - looks like the !len check is now done in vfs_splice_read(), but I didn't work out what happened to ppos and s_maxbytes; the hwpoison and "part" mods still needed, surely? Hugh
--- a/mm/shmem.c +++ b/mm/shmem.c @@ -2902,6 +2902,11 @@ static ssize_t shmem_file_splice_read(struct file *in, loff_t *ppos, loff_t isize; int error = 0; + if (unlikely(*ppos >= MAX_LFS_FILESIZE)) + return 0; + if (unlikely(!len)) + return 0; + /* Work out how much data we can actually add into the pipe */ used = pipe_occupancy(pipe->head, pipe->tail); npages = max_t(ssize_t, pipe->max_usage - used, 0); @@ -2911,7 +2916,8 @@ static ssize_t shmem_file_splice_read(struct file *in, loff_t *ppos, if (*ppos >= i_size_read(inode)) break; - error = shmem_get_folio(inode, *ppos / PAGE_SIZE, &folio, SGP_READ); + error = shmem_get_folio(inode, *ppos / PAGE_SIZE, &folio, + SGP_READ); if (error) { if (error == -EINVAL) error = 0; @@ -2920,7 +2926,9 @@ static ssize_t shmem_file_splice_read(struct file *in, loff_t *ppos, if (folio) { folio_unlock(folio); - if (folio_test_hwpoison(folio)) { + if (folio_test_hwpoison(folio) || + (folio_test_large(folio) && + folio_test_has_hwpoisoned(folio))) { error = -EIO; break; } @@ -2956,7 +2964,7 @@ static ssize_t shmem_file_splice_read(struct file *in, loff_t *ppos, folio_put(folio); folio = NULL; } else { - n = splice_zeropage_into_pipe(pipe, *ppos, len); + n = splice_zeropage_into_pipe(pipe, *ppos, part); } if (!n)
generic_file_splice_read() makes a couple of preliminary checks (for s_maxbytes and zero len), but shmem_file_splice_read() is called without those: so check them inside it. (But shmem does not support O_DIRECT, so no need for that one here - and even if O_DIRECT support were stubbed in, it would still just be using the page cache.) HWPoison: my reading of folio_test_hwpoison() is that it only tests the head page of a large folio, whereas splice_folio_into_pipe() will splice as much of the folio as it can: so for safety we should also check the has_hwpoisoned flag, set if any of the folio's pages are hwpoisoned. (Perhaps that ugliness can be improved at the mm end later.) The call to splice_zeropage_into_pipe() risked overrunning past EOF: ask it for "part" not "len". Fixes: b81d7b89becc ("shmem: Implement splice-read") Signed-off-by: Hugh Dickins <hughd@google.com> --- Thank you, David, for attending to tmpfs in your splice update: yes, I too wish it could have just used the generic, but I'm sure you're right that there's a number of reasons it needs its own. mm/shmem.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)