Re: [PATCH 1/6] seq_file: add seq_read_iter
On Tue, Dec 8, 2020 at 12:53 PM Al Viro wrote: > > Umm... I've got > fs: Handle I_DONTCACHE in iput_final() instead of generic_drop_inode() > and > fs: Kill DCACHE_DONTCACHE dentry even if DCACHE_REFERENCED is set > in "to apply" pile; if that's what you are talking about, Yup, those were the ones. > I don't > think they are anywhere critical enough for 5.10-final, but I might > be missing something... No, I agree, no hurry with them. I just wanted to make sure you're aware of them. Linus
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Tue, Dec 08, 2020 at 12:25:55PM -0800, Linus Torvalds wrote: > On Tue, Dec 8, 2020 at 11:49 AM Al Viro wrote: > > > > Said that, it does appear to survive all beating, and it does fix > > a regression introduced in this cycle, so, provided that amount of > > comments in there is OK with you... > > Ok, considering Greg's note, I've pulled it. It's early in the last > week, if something comes up we can still fix it. > > That said, considering that I think the only use-case was that odd > /proc splice use, and the really special WSL2 thing, and both of those > are verified, it does sound safe to pull. > > Famous last words... > > Al, since you're around, would you mind looking at the two > DCACHE_DONTCACHE patches too? Honestly, since they seem to be an issue > only for DAX, and only for DAX policy changes, I don't consider them > critical for 5.10, but they've been around for a while now. Umm... I've got fs: Handle I_DONTCACHE in iput_final() instead of generic_drop_inode() and fs: Kill DCACHE_DONTCACHE dentry even if DCACHE_REFERENCED is set in "to apply" pile; if that's what you are talking about, I don't think they are anywhere critical enough for 5.10-final, but I might be missing something... Al, still buried under piles of email ;-/
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Tue, Dec 8, 2020 at 11:49 AM Al Viro wrote: > > Said that, it does appear to survive all beating, and it does fix > a regression introduced in this cycle, so, provided that amount of > comments in there is OK with you... Ok, considering Greg's note, I've pulled it. It's early in the last week, if something comes up we can still fix it. That said, considering that I think the only use-case was that odd /proc splice use, and the really special WSL2 thing, and both of those are verified, it does sound safe to pull. Famous last words... Al, since you're around, would you mind looking at the two DCACHE_DONTCACHE patches too? Honestly, since they seem to be an issue only for DAX, and only for DAX policy changes, I don't consider them critical for 5.10, but they've been around for a while now. Linus
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Tue, Dec 08, 2020 at 10:34:45AM -0800, Linus Torvalds wrote: > On Tue, Dec 8, 2020 at 8:35 AM Christoph Hellwig wrote: > > > > > > Shouldn't this go to Linus before v5.10 is released? > > > > ping? > > So by now I'm a bit worried about this series, because the early fixes > caused more problems than the current state. > > So considering the timing and Al having been spotty, I think this is > post-5.10 and marked for stable. If you want some sort of "do these really work" validation, these have been running for a while now in the android 5.10-rc kernels just fine, as I cherry-picked the patches there to get past their testing issues. But if you want to wait until after 5.10 is out, that's fine with me too, it's up to Al. thanks, greg k-h
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Tue, Dec 08, 2020 at 10:34:45AM -0800, Linus Torvalds wrote: > On Tue, Dec 8, 2020 at 8:35 AM Christoph Hellwig wrote: > > > > > > Shouldn't this go to Linus before v5.10 is released? > > > > ping? > > So by now I'm a bit worried about this series, because the early fixes > caused more problems than the current state. *nod* Said that, it does appear to survive all beating, and it does fix a regression introduced in this cycle, so, provided that amount of comments in there is OK with you... The following changes since commit d4d50710a8b46082224376ef119a4dbb75b25c56: seq_file: add seq_read_iter (2020-11-06 10:05:18 -0800) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git fixes for you to fetch changes up to 4bbf439b09c5ac3f8b3e9584fe080375d8d0ad2d: fix return values of seq_read_iter() (2020-11-15 22:12:53 -0500) Al Viro (1): fix return values of seq_read_iter() fs/seq_file.c | 57 +++-- 1 file changed, 27 insertions(+), 30 deletions(-)
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Tue, Dec 8, 2020 at 8:35 AM Christoph Hellwig wrote: > > > > Shouldn't this go to Linus before v5.10 is released? > > ping? So by now I'm a bit worried about this series, because the early fixes caused more problems than the current state. So considering the timing and Al having been spotty, I think this is post-5.10 and marked for stable. Al, I'm willing to be convinced otherwise, but you need to respond.. Linus
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Fri, Nov 27, 2020 at 05:29:02PM +0100, Christoph Hellwig wrote: > On Mon, Nov 16, 2020 at 03:29:42AM +, Al Viro wrote: > > > Still good. > > > > > > Tested-by: Nathan Chancellor > > > > Pushed into #fixes > > Shouldn't this go to Linus before v5.10 is released? ping?
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Mon, Nov 16, 2020 at 03:29:42AM +, Al Viro wrote: > > Still good. > > > > Tested-by: Nathan Chancellor > > Pushed into #fixes Shouldn't this go to Linus before v5.10 is released?
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Sun, Nov 15, 2020 at 05:34:16PM -0700, Nathan Chancellor wrote:
> Still good.
>
> Tested-by: Nathan Chancellor
Pushed into #fixes
> > BTW, is that call of readv() really coming from init? And if it
> > is, what version of init are you using?
>
> I believe that it is but since this is WSL2, I believe that /init is a
> proprietary Microsoft implementation, rather than systemd or another
> init system:
>
> https://wiki.ubuntu.com/WSL#Keeping_Ubuntu_Updated_in_WSL
>
> So I am not sure how possible it is to see exactly what is going on or
> getting it improved.
Oh, well... Anyway, as a regression test it's interesting:
#include
#include
#include
#include
main()
{
static char s[1024];
static struct iovec v[2] = {{NULL, 0}, {s, 1024}};
for(;;) {
ssize_t n = readv(0, v, 2), m, w;
if (n < 0) {
perror("readv");
return -1;
}
if (!n)
return 0;
for (m = 0; m < n; m += w) {
w = write(1, s + m, n - m);
if (w < 0)
perror("write");
}
}
}
which ought to copy stdin to stdout; with this bug it would (on sufficiently
large seq_file-based files) fail with "readv: Bad address" (-EFAULT, that is).
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Mon, Nov 16, 2020 at 12:25:13AM +, Al Viro wrote:
> On Sun, Nov 15, 2020 at 04:51:49PM -0700, Nathan Chancellor wrote:
> > Looks good to me on top of d4d50710a8b46082224376ef119a4dbb75b25c56,
> > thanks for quickly looking into this!
> >
> > Tested-by: Nathan Chancellor
>
> OK... a variant with (hopefully) better comments and cleaned up
> logics in the second loop (
> if (seq_has_overflowed(m) || err) {
> m->count = offs;
> if (likely(err <= 0))
> break;
> }
> replaced with
> if (err > 0) { // ->show() says "skip it"
> m->count = offs;
> } else if (err || seq_has_overflowed(m)) {
> m->count = offs;
> break;
> }
> ) follows. I'm quite certain that it is an equivalent transformation
> (seq_has_overflowed() has no side effects) and IMO it's slightly
> more readable that way. Survives local beating; could you check if
> it's still OK with your testcase? Equivalent transformation or not,
> I'd rather not slap anyone's Tested-by: on a modified variant of
> patch...
Still good.
Tested-by: Nathan Chancellor
> BTW, is that call of readv() really coming from init? And if it
> is, what version of init are you using?
I believe that it is but since this is WSL2, I believe that /init is a
proprietary Microsoft implementation, rather than systemd or another
init system:
https://wiki.ubuntu.com/WSL#Keeping_Ubuntu_Updated_in_WSL
So I am not sure how possible it is to see exactly what is going on or
getting it improved.
Cheers,
Nathan
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Sun, Nov 15, 2020 at 04:51:49PM -0700, Nathan Chancellor wrote:
> Looks good to me on top of d4d50710a8b46082224376ef119a4dbb75b25c56,
> thanks for quickly looking into this!
>
> Tested-by: Nathan Chancellor
OK... a variant with (hopefully) better comments and cleaned up
logics in the second loop (
if (seq_has_overflowed(m) || err) {
m->count = offs;
if (likely(err <= 0))
break;
}
replaced with
if (err > 0) { // ->show() says "skip it"
m->count = offs;
} else if (err || seq_has_overflowed(m)) {
m->count = offs;
break;
}
) follows. I'm quite certain that it is an equivalent transformation
(seq_has_overflowed() has no side effects) and IMO it's slightly
more readable that way. Survives local beating; could you check if
it's still OK with your testcase? Equivalent transformation or not,
I'd rather not slap anyone's Tested-by: on a modified variant of
patch...
BTW, is that call of readv() really coming from init? And if it
is, what version of init are you using?
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3b20e21604e7..03a369ccd28c 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -168,12 +168,14 @@ EXPORT_SYMBOL(seq_read);
ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
{
struct seq_file *m = iocb->ki_filp->private_data;
- size_t size = iov_iter_count(iter);
size_t copied = 0;
size_t n;
void *p;
int err = 0;
+ if (!iov_iter_count(iter))
+ return 0;
+
mutex_lock(>lock);
/*
@@ -206,36 +208,34 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
*iter)
if (!m->buf)
goto Enomem;
}
- /* if not empty - flush it first */
+ // something left in the buffer - copy it out first
if (m->count) {
- n = min(m->count, size);
- if (copy_to_iter(m->buf + m->from, n, iter) != n)
- goto Efault;
+ n = copy_to_iter(m->buf + m->from, m->count, iter);
m->count -= n;
m->from += n;
- size -= n;
copied += n;
- if (!size)
+ if (m->count) // hadn't managed to copy everything
goto Done;
}
- /* we need at least one record in buffer */
+ // get a non-empty record in the buffer
m->from = 0;
p = m->op->start(m, >index);
while (1) {
err = PTR_ERR(p);
- if (!p || IS_ERR(p))
+ if (!p || IS_ERR(p))// EOF or an error
break;
err = m->op->show(m, p);
- if (err < 0)
+ if (err < 0)// hard error
break;
- if (unlikely(err))
+ if (unlikely(err)) // ->show() says "skip it"
m->count = 0;
- if (unlikely(!m->count)) {
+ if (unlikely(!m->count)) { // empty record
p = m->op->next(m, p, >index);
continue;
}
- if (m->count < m->size)
+ if (!seq_has_overflowed(m)) // got it
goto Fill;
+ // need a bigger buffer
m->op->stop(m, p);
kvfree(m->buf);
m->count = 0;
@@ -244,11 +244,14 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
*iter)
goto Enomem;
p = m->op->start(m, >index);
}
+ // EOF or an error
m->op->stop(m, p);
m->count = 0;
goto Done;
Fill:
- /* they want more? let's try to get some more */
+ // one non-empty record is in the buffer; if they want more,
+ // try to fit more in, but in any case we need to advance
+ // the iterator once for every record shown.
while (1) {
size_t offs = m->count;
loff_t pos = m->index;
@@ -259,30 +262,27 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
*iter)
m->op->next);
m->index++;
}
- if (!p || IS_ERR(p)) {
- err = PTR_ERR(p);
+ if (!p || IS_ERR(p))// no next record for us
break;
- }
- if (m->count >= size)
+ if (m->count >= iov_iter_count(iter))
break;
err = m->op->show(m, p);
- if (seq_has_overflowed(m) || err) {
+ if (err > 0) { // ->show() says "skip it"
m->count = offs;
- if
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Sun, Nov 15, 2020 at 11:38:14PM +, Al Viro wrote:
> On Sun, Nov 15, 2020 at 02:41:25PM -0700, Nathan Chancellor wrote:
> > Hi Al,
> >
> > Apologies for the delay.
> >
> > On Sun, Nov 15, 2020 at 03:53:55PM +, Al Viro wrote:
> > > On Sat, Nov 14, 2020 at 08:50:00PM +, Al Viro wrote:
> > >
> > > OK, I think I understand what's going on. Could you check if
> > > reverting the variant in -next and applying the following instead
> > > fixes what you are seeing?
> >
> > The below diff on top of d4d50710a8b46082224376ef119a4dbb75b25c56 does
> > not fix my issue unfortunately.
>
> OK... Now that I have a reproducer[1], I think I've sorted it out.
> And yes, it had been too subtle for its own good ;-/
>
> [1] I still wonder what the hell in the userland has come up with the
> idea of reading through a file with readv(), each time with 2-element
> iovec array, the first element covering 0 bytes and the second one - 1024.
> AFAICS, nothing is systemd git appears to be _that_ weird... Makes for
> a useful testcase, though...
>
> Anyway, could you test this replacement?
Looks good to me on top of d4d50710a8b46082224376ef119a4dbb75b25c56,
thanks for quickly looking into this!
Tested-by: Nathan Chancellor
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 3b20e21604e7..c0dfe2861b35 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -168,12 +168,14 @@ EXPORT_SYMBOL(seq_read);
> ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> {
> struct seq_file *m = iocb->ki_filp->private_data;
> - size_t size = iov_iter_count(iter);
> size_t copied = 0;
> size_t n;
> void *p;
> int err = 0;
>
> + if (!iov_iter_count(iter))
> + return 0;
> +
> mutex_lock(>lock);
>
> /*
> @@ -208,34 +210,32 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct
> iov_iter *iter)
> }
> /* if not empty - flush it first */
> if (m->count) {
> - n = min(m->count, size);
> - if (copy_to_iter(m->buf + m->from, n, iter) != n)
> - goto Efault;
> + n = copy_to_iter(m->buf + m->from, m->count, iter);
> m->count -= n;
> m->from += n;
> - size -= n;
> copied += n;
> - if (!size)
> + if (m->count) // hadn't managed to copy everything
> goto Done;
> }
> - /* we need at least one record in buffer */
> + /* we need at least one non-empty record in the buffer */
> m->from = 0;
> p = m->op->start(m, >index);
> while (1) {
> err = PTR_ERR(p);
> - if (!p || IS_ERR(p))
> + if (!p || IS_ERR(p))// EOF or an error
> break;
> err = m->op->show(m, p);
> - if (err < 0)
> + if (err < 0)// hard error
> break;
> - if (unlikely(err))
> + if (unlikely(err)) // ->show() says "skip it"
> m->count = 0;
> - if (unlikely(!m->count)) {
> + if (unlikely(!m->count)) { // empty record
> p = m->op->next(m, p, >index);
> continue;
> }
> - if (m->count < m->size)
> + if (!seq_has_overflowed(m)) // got it
> goto Fill;
> + // need a bigger buffer
> m->op->stop(m, p);
> kvfree(m->buf);
> m->count = 0;
> @@ -244,11 +244,14 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct
> iov_iter *iter)
> goto Enomem;
> p = m->op->start(m, >index);
> }
> + // EOF or an error
> m->op->stop(m, p);
> m->count = 0;
> goto Done;
> Fill:
> - /* they want more? let's try to get some more */
> + // one non-empty record is in the buffer; if they want more,
> + // try to fit more in, but in any case we need to advance
> + // the iterator at least once.
> while (1) {
> size_t offs = m->count;
> loff_t pos = m->index;
> @@ -259,11 +262,9 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct
> iov_iter *iter)
> m->op->next);
> m->index++;
> }
> - if (!p || IS_ERR(p)) {
> - err = PTR_ERR(p);
> + if (!p || IS_ERR(p))// no next record for us
> break;
> - }
> - if (m->count >= size)
> + if (m->count >= iov_iter_count(iter))
> break;
> err = m->op->show(m, p);
> if (seq_has_overflowed(m) || err) {
> @@ -273,16 +274,14 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct
> iov_iter *iter)
> }
> }
> m->op->stop(m, p);
> - n = min(m->count, size);
> - if
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Sun, Nov 15, 2020 at 02:41:25PM -0700, Nathan Chancellor wrote:
> Hi Al,
>
> Apologies for the delay.
>
> On Sun, Nov 15, 2020 at 03:53:55PM +, Al Viro wrote:
> > On Sat, Nov 14, 2020 at 08:50:00PM +, Al Viro wrote:
> >
> > OK, I think I understand what's going on. Could you check if
> > reverting the variant in -next and applying the following instead
> > fixes what you are seeing?
>
> The below diff on top of d4d50710a8b46082224376ef119a4dbb75b25c56 does
> not fix my issue unfortunately.
OK... Now that I have a reproducer[1], I think I've sorted it out.
And yes, it had been too subtle for its own good ;-/
[1] I still wonder what the hell in the userland has come up with the
idea of reading through a file with readv(), each time with 2-element
iovec array, the first element covering 0 bytes and the second one - 1024.
AFAICS, nothing is systemd git appears to be _that_ weird... Makes for
a useful testcase, though...
Anyway, could you test this replacement?
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3b20e21604e7..c0dfe2861b35 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -168,12 +168,14 @@ EXPORT_SYMBOL(seq_read);
ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
{
struct seq_file *m = iocb->ki_filp->private_data;
- size_t size = iov_iter_count(iter);
size_t copied = 0;
size_t n;
void *p;
int err = 0;
+ if (!iov_iter_count(iter))
+ return 0;
+
mutex_lock(>lock);
/*
@@ -208,34 +210,32 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
*iter)
}
/* if not empty - flush it first */
if (m->count) {
- n = min(m->count, size);
- if (copy_to_iter(m->buf + m->from, n, iter) != n)
- goto Efault;
+ n = copy_to_iter(m->buf + m->from, m->count, iter);
m->count -= n;
m->from += n;
- size -= n;
copied += n;
- if (!size)
+ if (m->count) // hadn't managed to copy everything
goto Done;
}
- /* we need at least one record in buffer */
+ /* we need at least one non-empty record in the buffer */
m->from = 0;
p = m->op->start(m, >index);
while (1) {
err = PTR_ERR(p);
- if (!p || IS_ERR(p))
+ if (!p || IS_ERR(p))// EOF or an error
break;
err = m->op->show(m, p);
- if (err < 0)
+ if (err < 0)// hard error
break;
- if (unlikely(err))
+ if (unlikely(err)) // ->show() says "skip it"
m->count = 0;
- if (unlikely(!m->count)) {
+ if (unlikely(!m->count)) { // empty record
p = m->op->next(m, p, >index);
continue;
}
- if (m->count < m->size)
+ if (!seq_has_overflowed(m)) // got it
goto Fill;
+ // need a bigger buffer
m->op->stop(m, p);
kvfree(m->buf);
m->count = 0;
@@ -244,11 +244,14 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
*iter)
goto Enomem;
p = m->op->start(m, >index);
}
+ // EOF or an error
m->op->stop(m, p);
m->count = 0;
goto Done;
Fill:
- /* they want more? let's try to get some more */
+ // one non-empty record is in the buffer; if they want more,
+ // try to fit more in, but in any case we need to advance
+ // the iterator at least once.
while (1) {
size_t offs = m->count;
loff_t pos = m->index;
@@ -259,11 +262,9 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
*iter)
m->op->next);
m->index++;
}
- if (!p || IS_ERR(p)) {
- err = PTR_ERR(p);
+ if (!p || IS_ERR(p))// no next record for us
break;
- }
- if (m->count >= size)
+ if (m->count >= iov_iter_count(iter))
break;
err = m->op->show(m, p);
if (seq_has_overflowed(m) || err) {
@@ -273,16 +274,14 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
*iter)
}
}
m->op->stop(m, p);
- n = min(m->count, size);
- if (copy_to_iter(m->buf, n, iter) != n)
- goto Efault;
+ n = copy_to_iter(m->buf, m->count, iter);
copied += n;
m->count -= n;
m->from = n;
Done:
- if (!copied)
- copied = err;
- else {
+ if (unlikely(!copied)) {
+
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Sun, Nov 15, 2020 at 7:54 AM Al Viro wrote: > > OK, I think I understand what's going on. Could you check if > reverting the variant in -next and applying the following instead > fixes what you are seeing? Side note: if this ends up working, can you add a lot of comments about this thing (both in the code and the commit message)? It confused both Christoph and me, and clearly you were stumped too. That's not a great sign. Linus
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Sat, Nov 14, 2020 at 08:50:00PM +, Al Viro wrote:
OK, I think I understand what's going on. Could you check if
reverting the variant in -next and applying the following instead
fixes what you are seeing?
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3b20e21604e7..35667112bbd1 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -168,7 +168,6 @@ EXPORT_SYMBOL(seq_read);
ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
{
struct seq_file *m = iocb->ki_filp->private_data;
- size_t size = iov_iter_count(iter);
size_t copied = 0;
size_t n;
void *p;
@@ -208,16 +207,15 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
*iter)
}
/* if not empty - flush it first */
if (m->count) {
- n = min(m->count, size);
- if (copy_to_iter(m->buf + m->from, n, iter) != n)
- goto Efault;
+ n = copy_to_iter(m->buf + m->from, m->count, iter);
m->count -= n;
m->from += n;
- size -= n;
copied += n;
- if (!size)
- goto Done;
+ if (m->count)
+ goto Efault;
}
+ if (!iov_iter_count(iter))
+ goto Done;
/* we need at least one record in buffer */
m->from = 0;
p = m->op->start(m, >index);
@@ -249,6 +247,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
*iter)
goto Done;
Fill:
/* they want more? let's try to get some more */
+ /* m->count is positive and there's space left in iter */
while (1) {
size_t offs = m->count;
loff_t pos = m->index;
@@ -263,7 +262,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
*iter)
err = PTR_ERR(p);
break;
}
- if (m->count >= size)
+ if (m->count >= iov_iter_count(iter))
break;
err = m->op->show(m, p);
if (seq_has_overflowed(m) || err) {
@@ -273,12 +272,12 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
*iter)
}
}
m->op->stop(m, p);
- n = min(m->count, size);
- if (copy_to_iter(m->buf, n, iter) != n)
- goto Efault;
+ n = copy_to_iter(m->buf, m->count, iter);
copied += n;
- m->count -= n;
m->from = n;
+ m->count -= n;
+ if (m->count)
+ goto Efault;
Done:
if (!copied)
copied = err;
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Fri, Nov 13, 2020 at 04:54:53PM -0700, Nathan Chancellor wrote:
> Hi Al,
>
> On Wed, Nov 11, 2020 at 10:21:16PM +, Al Viro wrote:
> > On Wed, Nov 11, 2020 at 09:52:20PM +, Al Viro wrote:
> >
> > > That can be done, but I would rather go with
> > > n = copy_to_iter(m->buf + m->from, m->count, iter);
> > > m->count -= n;
> > > m->from += n;
> > > copied += n;
> > > if (!size)
> > > goto Done;
> > > if (m->count)
> > > goto Efault;
> > > if we do it that way. Let me see if I can cook something
> > > reasonable along those lines...
> >
> > Something like below (build-tested only):
> >
> > diff --git a/fs/seq_file.c b/fs/seq_file.c
> > index 3b20e21604e7..07b33c1f34a9 100644
> > --- a/fs/seq_file.c
> > +++ b/fs/seq_file.c
> > @@ -168,7 +168,6 @@ EXPORT_SYMBOL(seq_read);
> > ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> > {
> > struct seq_file *m = iocb->ki_filp->private_data;
> > - size_t size = iov_iter_count(iter);
> > size_t copied = 0;
> > size_t n;
> > void *p;
> > @@ -208,14 +207,11 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct
> > iov_iter *iter)
> > }
> > /* if not empty - flush it first */
> > if (m->count) {
> > - n = min(m->count, size);
> > - if (copy_to_iter(m->buf + m->from, n, iter) != n)
> > - goto Efault;
> > + n = copy_to_iter(m->buf + m->from, m->count, iter);
> > m->count -= n;
> > m->from += n;
> > - size -= n;
> > copied += n;
> > - if (!size)
> > + if (!iov_iter_count(iter) || m->count)
> > goto Done;
> > }
> > /* we need at least one record in buffer */
> > @@ -249,6 +245,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct
> > iov_iter *iter)
> > goto Done;
> > Fill:
> > /* they want more? let's try to get some more */
> > + /* m->count is positive and there's space left in iter */
> > while (1) {
> > size_t offs = m->count;
> > loff_t pos = m->index;
> > @@ -263,7 +260,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct
> > iov_iter *iter)
> > err = PTR_ERR(p);
> > break;
> > }
> > - if (m->count >= size)
> > + if (m->count >= iov_iter_count(iter))
> > break;
> > err = m->op->show(m, p);
> > if (seq_has_overflowed(m) || err) {
> > @@ -273,16 +270,14 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct
> > iov_iter *iter)
> > }
> > }
> > m->op->stop(m, p);
> > - n = min(m->count, size);
> > - if (copy_to_iter(m->buf, n, iter) != n)
> > - goto Efault;
> > + n = copy_to_iter(m->buf, m->count, iter);
> > copied += n;
> > m->count -= n;
> > m->from = n;
> > Done:
> > - if (!copied)
> > - copied = err;
> > - else {
> > + if (unlikely(!copied)) {
> > + copied = m->count ? -EFAULT : err;
> > + } else {
> > iocb->ki_pos += copied;
> > m->read_pos += copied;
> > }
> > @@ -291,9 +286,6 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct
> > iov_iter *iter)
> > Enomem:
> > err = -ENOMEM;
> > goto Done;
> > -Efault:
> > - err = -EFAULT;
> > - goto Done;
> > }
> > EXPORT_SYMBOL(seq_read_iter);
> >
>
> This patch in -next (6a9f696d1627bacc91d1cebcfb177f474484e8ba) breaks
> WSL2's interoperability feature, where Windows paths automatically get
> added to PATH on start up so that Windows binaries can be accessed from
> within Linux (such as clip.exe to pipe output to the clipboard). Before,
> I would see a bunch of Linux + Windows folders in $PATH but after, I
> only see the Linux folders (I can give you the actual PATH value if you
> care but it is really long).
>
> I am not at all familiar with the semantics of this patch or how
> Microsoft would be using it to inject folders into PATH (they have some
> documentation on it here:
> https://docs.microsoft.com/en-us/windows/wsl/interop) and I am not sure
> how to go about figuring that out to see why this patch breaks something
> (unless you have an idea). I have added the Hyper-V maintainers and list
> to CC in case they know someone who could help.
FWIW, just to make sure:
1) does reverting just that commit recover the desired behaviour?
2) could you verify that your latest tests had been done with
the incremental I'd posted (shifting the if () goto Done; out of the if
body)?
3) does the build with that commit reverted produce any warnings
related to mountinfo?
4) your posted log with WARN_ON unfortunately starts *after*
the mountinfo accesses; could you check which process had been doing those?
The Comm: ... part in there, that is.
5) in the "I don't believe that could happen, but let's make sure"
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Sat, Nov 14, 2020 at 07:00:25AM +, Al Viro wrote:
> On Fri, Nov 13, 2020 at 11:19:34PM -0700, Nathan Chancellor wrote:
>
> > Assuming so, I have attached the output both with and without the
> > WARN_ON. Looks like mountinfo is what is causing the error?
>
> Cute... FWIW, on #origin + that commit with fix folded in I don't
> see anything unusual in reads from mountinfo ;-/ OTOH, they'd
> obviously been... creative with readv(2) arguments, so it would
> be very interesting to see what it is they are passing to it.
>
> I'm half-asleep right now; will try to cook something to gather
> that information tomorrow morning. 'Later...
OK, so let's do this: fix in seq_read_iter() + in do_loop_readv_writev()
(on entry) the following (racy as hell, but will do for debugging):
bool weird = false;
if (unlikely(memcmp(file->f_path.dentry->d_name.name, "mountinfo",
10))) {
int i;
for (i = 0; i < iter->nr_segs; i++)
if (!iter->iov[i].iov_len)
weird = true;
if (weird) {
printk(KERN_ERR "[%s]: weird readv on %p4D (%ld) ",
current->comm, filp, (long)filp->f_pos);
for (i = 0; i < iter->nr_segs; i++)
printk(KERN_CONT "%c%zd", i ? ':' : '<',
iter->iov[i].iov_len);
printk(KERN_CONT "> ");
}
}
and in the end (just before return)
if (weird)
printk(KERN_CONT "-> %zd\n", ret);
Preferably along with the results of cat /proc//mountinfo both
on that and on the working kernel...
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Fri, Nov 13, 2020 at 11:19:34PM -0700, Nathan Chancellor wrote: > Assuming so, I have attached the output both with and without the > WARN_ON. Looks like mountinfo is what is causing the error? Cute... FWIW, on #origin + that commit with fix folded in I don't see anything unusual in reads from mountinfo ;-/ OTOH, they'd obviously been... creative with readv(2) arguments, so it would be very interesting to see what it is they are passing to it. I'm half-asleep right now; will try to cook something to gather that information tomorrow morning. 'Later...
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Fri, Nov 13, 2020 at 09:14:20PM -0700, Nathan Chancellor wrote: > Unfortunately that patch does not solve my issue. Is there any other > debugging I should add? Hmm... I wonder which file it is; how about if (WARN_ON(!iovec.iov_len)) printk(KERN_ERR "odd readv on %pd4\n", file); in the loop in fs/read_write.c:do_loop_readv_writev()?
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Sat, Nov 14, 2020 at 03:54:53AM +, Al Viro wrote: > On Fri, Nov 13, 2020 at 08:01:24PM -0700, Nathan Chancellor wrote: > > Sure thing, it does trigger. > > > > [0.235058] [ cut here ] > > [0.235062] WARNING: CPU: 15 PID: 237 at fs/seq_file.c:176 > > seq_read_iter+0x3b3/0x3f0 > > [0.235064] CPU: 15 PID: 237 Comm: localhost Not tainted > > 5.10.0-rc2-microsoft-cbl-2-g6a9f696d1627-dirty #15 > > [0.235065] RIP: 0010:seq_read_iter+0x3b3/0x3f0 > > [0.235066] Code: ba 01 00 00 00 e8 6d d2 fc ff 4c 89 e7 48 89 ee 48 8b > > 54 24 10 e8 ad 8b 45 00 49 01 c5 48 29 43 18 48 89 43 10 e9 61 fe ff ff > > <0f> 0b e9 6f fc ff ff 0f 0b 45 31 ed e9 0d fd ff ff 48 c7 43 18 00 > > [0.235067] RSP: 0018:9c774063bd08 EFLAGS: 00010246 > > [0.235068] RAX: 91a77ac01f00 RBX: 91a50133c348 RCX: > > 0001 > > [0.235069] RDX: 9c774063bdb8 RSI: 9c774063bd60 RDI: > > 9c774063bd88 > > [0.235069] RBP: R08: R09: > > 91a50058b768 > > [0.235070] R10: 91a7f79f R11: bc2c2030 R12: > > 9c774063bd88 > > [0.235070] R13: 9c774063bd60 R14: 9c774063be48 R15: > > 91a77af58900 > > [0.235072] FS: 0029c800() GS:91a7f7bc() > > knlGS: > > [0.235073] CS: 0010 DS: ES: CR0: 80050033 > > [0.235073] CR2: 7ab6c1fabad0 CR3: 00037a004000 CR4: > > 00350ea0 > > [0.235074] Call Trace: > > [0.235077] seq_read+0x127/0x150 > > [0.235078] proc_reg_read+0x42/0xa0 > > [0.235080] do_iter_read+0x14c/0x1e0 > > [0.235081] do_readv+0x18d/0x240 > > [0.235083] do_syscall_64+0x33/0x70 > > [0.235085] entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > *blink* > > Lovely... For one thing, it did *not* go through > proc_reg_read_iter(). For another, it has hit proc_reg_read() with > zero length, which must've been an iovec with zero ->iov_len in > readv(2) arguments. I wonder if we should use that kind of > pathology (readv() with zero-length segment in the middle of > iovec array) for regression tests... > > OK... First of all, since that kind of crap can happen, > let's do this (incremental to be folded); then (and that's > a separate patch) we ought to switch the proc_ops with ->proc_read > equal to seq_read to ->proc_read_iter = seq_read_iter, so that > those guys would not mess with seq_read() wrapper at all. > > Finally, is there any point having do_loop_readv_writev() > call any methods for zero-length segments? > > In any case, the following should be folded into > "fix return values of seq_read_iter()"; could you check if that > fixes the problem you are seeing? > > diff --git a/fs/seq_file.c b/fs/seq_file.c > index 07b33c1f34a9..e66d6b8bae23 100644 > --- a/fs/seq_file.c > +++ b/fs/seq_file.c > @@ -211,9 +211,9 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter > *iter) > m->count -= n; > m->from += n; > copied += n; > - if (!iov_iter_count(iter) || m->count) > - goto Done; > } > + if (m->count || !iov_iter_count(iter)) > + goto Done; > /* we need at least one record in buffer */ > m->from = 0; > p = m->op->start(m, >index); Unfortunately that patch does not solve my issue. Is there any other debugging I should add? Cheers, Nathan
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Fri, Nov 13, 2020 at 08:01:24PM -0700, Nathan Chancellor wrote: > Sure thing, it does trigger. > > [0.235058] [ cut here ] > [0.235062] WARNING: CPU: 15 PID: 237 at fs/seq_file.c:176 > seq_read_iter+0x3b3/0x3f0 > [0.235064] CPU: 15 PID: 237 Comm: localhost Not tainted > 5.10.0-rc2-microsoft-cbl-2-g6a9f696d1627-dirty #15 > [0.235065] RIP: 0010:seq_read_iter+0x3b3/0x3f0 > [0.235066] Code: ba 01 00 00 00 e8 6d d2 fc ff 4c 89 e7 48 89 ee 48 8b 54 > 24 10 e8 ad 8b 45 00 49 01 c5 48 29 43 18 48 89 43 10 e9 61 fe ff ff <0f> 0b > e9 6f fc ff ff 0f 0b 45 31 ed e9 0d fd ff ff 48 c7 43 18 00 > [0.235067] RSP: 0018:9c774063bd08 EFLAGS: 00010246 > [0.235068] RAX: 91a77ac01f00 RBX: 91a50133c348 RCX: > 0001 > [0.235069] RDX: 9c774063bdb8 RSI: 9c774063bd60 RDI: > 9c774063bd88 > [0.235069] RBP: R08: R09: > 91a50058b768 > [0.235070] R10: 91a7f79f R11: bc2c2030 R12: > 9c774063bd88 > [0.235070] R13: 9c774063bd60 R14: 9c774063be48 R15: > 91a77af58900 > [0.235072] FS: 0029c800() GS:91a7f7bc() > knlGS: > [0.235073] CS: 0010 DS: ES: CR0: 80050033 > [0.235073] CR2: 7ab6c1fabad0 CR3: 00037a004000 CR4: > 00350ea0 > [0.235074] Call Trace: > [0.235077] seq_read+0x127/0x150 > [0.235078] proc_reg_read+0x42/0xa0 > [0.235080] do_iter_read+0x14c/0x1e0 > [0.235081] do_readv+0x18d/0x240 > [0.235083] do_syscall_64+0x33/0x70 > [0.235085] entry_SYSCALL_64_after_hwframe+0x44/0xa9 *blink* Lovely... For one thing, it did *not* go through proc_reg_read_iter(). For another, it has hit proc_reg_read() with zero length, which must've been an iovec with zero ->iov_len in readv(2) arguments. I wonder if we should use that kind of pathology (readv() with zero-length segment in the middle of iovec array) for regression tests... OK... First of all, since that kind of crap can happen, let's do this (incremental to be folded); then (and that's a separate patch) we ought to switch the proc_ops with ->proc_read equal to seq_read to ->proc_read_iter = seq_read_iter, so that those guys would not mess with seq_read() wrapper at all. Finally, is there any point having do_loop_readv_writev() call any methods for zero-length segments? In any case, the following should be folded into "fix return values of seq_read_iter()"; could you check if that fixes the problem you are seeing? diff --git a/fs/seq_file.c b/fs/seq_file.c index 07b33c1f34a9..e66d6b8bae23 100644 --- a/fs/seq_file.c +++ b/fs/seq_file.c @@ -211,9 +211,9 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter) m->count -= n; m->from += n; copied += n; - if (!iov_iter_count(iter) || m->count) - goto Done; } + if (m->count || !iov_iter_count(iter)) + goto Done; /* we need at least one record in buffer */ m->from = 0; p = m->op->start(m, >index);
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Sat, Nov 14, 2020 at 01:17:54AM +, Al Viro wrote: > On Fri, Nov 13, 2020 at 04:54:53PM -0700, Nathan Chancellor wrote: > > > This patch in -next (6a9f696d1627bacc91d1cebcfb177f474484e8ba) breaks > > WSL2's interoperability feature, where Windows paths automatically get > > added to PATH on start up so that Windows binaries can be accessed from > > within Linux (such as clip.exe to pipe output to the clipboard). Before, > > I would see a bunch of Linux + Windows folders in $PATH but after, I > > only see the Linux folders (I can give you the actual PATH value if you > > care but it is really long). > > > > I am not at all familiar with the semantics of this patch or how > > Microsoft would be using it to inject folders into PATH (they have some > > documentation on it here: > > https://docs.microsoft.com/en-us/windows/wsl/interop) and I am not sure > > how to go about figuring that out to see why this patch breaks something > > (unless you have an idea). I have added the Hyper-V maintainers and list > > to CC in case they know someone who could help. > > Out of curiosity: could you slap WARN_ON(!iov_iter_count(iter)); right in > the beginning of seq_read_iter() and see if that triggers? Sure thing, it does trigger. [0.235058] [ cut here ] [0.235062] WARNING: CPU: 15 PID: 237 at fs/seq_file.c:176 seq_read_iter+0x3b3/0x3f0 [0.235064] CPU: 15 PID: 237 Comm: localhost Not tainted 5.10.0-rc2-microsoft-cbl-2-g6a9f696d1627-dirty #15 [0.235065] RIP: 0010:seq_read_iter+0x3b3/0x3f0 [0.235066] Code: ba 01 00 00 00 e8 6d d2 fc ff 4c 89 e7 48 89 ee 48 8b 54 24 10 e8 ad 8b 45 00 49 01 c5 48 29 43 18 48 89 43 10 e9 61 fe ff ff <0f> 0b e9 6f fc ff ff 0f 0b 45 31 ed e9 0d fd ff ff 48 c7 43 18 00 [0.235067] RSP: 0018:9c774063bd08 EFLAGS: 00010246 [0.235068] RAX: 91a77ac01f00 RBX: 91a50133c348 RCX: 0001 [0.235069] RDX: 9c774063bdb8 RSI: 9c774063bd60 RDI: 9c774063bd88 [0.235069] RBP: R08: R09: 91a50058b768 [0.235070] R10: 91a7f79f R11: bc2c2030 R12: 9c774063bd88 [0.235070] R13: 9c774063bd60 R14: 9c774063be48 R15: 91a77af58900 [0.235072] FS: 0029c800() GS:91a7f7bc() knlGS: [0.235073] CS: 0010 DS: ES: CR0: 80050033 [0.235073] CR2: 7ab6c1fabad0 CR3: 00037a004000 CR4: 00350ea0 [0.235074] Call Trace: [0.235077] seq_read+0x127/0x150 [0.235078] proc_reg_read+0x42/0xa0 [0.235080] do_iter_read+0x14c/0x1e0 [0.235081] do_readv+0x18d/0x240 [0.235083] do_syscall_64+0x33/0x70 [0.235085] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [0.235086] RIP: 0033:0x22c483 [0.235086] Code: 4e 66 48 0f 7e c8 48 83 f8 01 48 89 d0 48 83 d0 ff 48 89 46 08 66 0f 7f 46 10 48 63 7f 78 b8 13 00 00 00 ba 02 00 00 00 0f 05 <48> 89 c7 e8 15 bb ff ff 48 85 c0 7e 34 48 89 c1 48 2b 4c 24 08 76 [0.235087] RSP: 002b:7ffca2245ca0 EFLAGS: 0257 ORIG_RAX: 0013 [0.235088] RAX: ffda RBX: 00a58120 RCX: 0022c483 [0.235088] RDX: 0002 RSI: 7ffca2245ca0 RDI: 0005 [0.235089] RBP: R08: fefefefefefefeff R09: 8080808080808080 [0.235089] R10: 7ab6c1fabb20 R11: 0257 R12: 00a58120 [0.235089] R13: 7ffca2245d90 R14: 0001 R15: 7ffca2245ce7 [0.235091] CPU: 15 PID: 237 Comm: localhost Not tainted 5.10.0-rc2-microsoft-cbl-2-g6a9f696d1627-dirty #15 [0.235091] Call Trace: [0.235092] dump_stack+0xa1/0xfb [0.235094] __warn+0x7f/0x120 [0.235095] ? seq_read_iter+0x3b3/0x3f0 [0.235096] report_bug+0xb1/0x110 [0.235097] handle_bug+0x3d/0x70 [0.235098] exc_invalid_op+0x18/0xb0 [0.235098] asm_exc_invalid_op+0x12/0x20 [0.235100] RIP: 0010:seq_read_iter+0x3b3/0x3f0 [0.235100] Code: ba 01 00 00 00 e8 6d d2 fc ff 4c 89 e7 48 89 ee 48 8b 54 24 10 e8 ad 8b 45 00 49 01 c5 48 29 43 18 48 89 43 10 e9 61 fe ff ff <0f> 0b e9 6f fc ff ff 0f 0b 45 31 ed e9 0d fd ff ff 48 c7 43 18 00 [0.235101] RSP: 0018:9c774063bd08 EFLAGS: 00010246 [0.235101] RAX: 91a77ac01f00 RBX: 91a50133c348 RCX: 0001 [0.235102] RDX: 9c774063bdb8 RSI: 9c774063bd60 RDI: 9c774063bd88 [0.235102] RBP: R08: R09: 91a50058b768 [0.235103] R10: 91a7f79f R11: bc2c2030 R12: 9c774063bd88 [0.235103] R13: 9c774063bd60 R14: 9c774063be48 R15: 91a77af58900 [0.235104] ? seq_open+0x70/0x70 [0.235105] ? path_openat+0xbc0/0xc40 [0.235106] seq_read+0x127/0x150 [0.235107] proc_reg_read+0x42/0xa0 [0.235108] do_iter_read+0x14c/0x1e0 [0.235109] do_readv+0x18d/0x240 [0.235109] do_syscall_64+0x33/0x70 [0.235110] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [0.235111] RIP:
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Fri, Nov 13, 2020 at 04:54:53PM -0700, Nathan Chancellor wrote: > This patch in -next (6a9f696d1627bacc91d1cebcfb177f474484e8ba) breaks > WSL2's interoperability feature, where Windows paths automatically get > added to PATH on start up so that Windows binaries can be accessed from > within Linux (such as clip.exe to pipe output to the clipboard). Before, > I would see a bunch of Linux + Windows folders in $PATH but after, I > only see the Linux folders (I can give you the actual PATH value if you > care but it is really long). > > I am not at all familiar with the semantics of this patch or how > Microsoft would be using it to inject folders into PATH (they have some > documentation on it here: > https://docs.microsoft.com/en-us/windows/wsl/interop) and I am not sure > how to go about figuring that out to see why this patch breaks something > (unless you have an idea). I have added the Hyper-V maintainers and list > to CC in case they know someone who could help. Out of curiosity: could you slap WARN_ON(!iov_iter_count(iter)); right in the beginning of seq_read_iter() and see if that triggers?
Re: [PATCH 1/6] seq_file: add seq_read_iter
Hi Al,
On Wed, Nov 11, 2020 at 10:21:16PM +, Al Viro wrote:
> On Wed, Nov 11, 2020 at 09:52:20PM +, Al Viro wrote:
>
> > That can be done, but I would rather go with
> > n = copy_to_iter(m->buf + m->from, m->count, iter);
> > m->count -= n;
> > m->from += n;
> > copied += n;
> > if (!size)
> > goto Done;
> > if (m->count)
> > goto Efault;
> > if we do it that way. Let me see if I can cook something
> > reasonable along those lines...
>
> Something like below (build-tested only):
>
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 3b20e21604e7..07b33c1f34a9 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -168,7 +168,6 @@ EXPORT_SYMBOL(seq_read);
> ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> {
> struct seq_file *m = iocb->ki_filp->private_data;
> - size_t size = iov_iter_count(iter);
> size_t copied = 0;
> size_t n;
> void *p;
> @@ -208,14 +207,11 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct
> iov_iter *iter)
> }
> /* if not empty - flush it first */
> if (m->count) {
> - n = min(m->count, size);
> - if (copy_to_iter(m->buf + m->from, n, iter) != n)
> - goto Efault;
> + n = copy_to_iter(m->buf + m->from, m->count, iter);
> m->count -= n;
> m->from += n;
> - size -= n;
> copied += n;
> - if (!size)
> + if (!iov_iter_count(iter) || m->count)
> goto Done;
> }
> /* we need at least one record in buffer */
> @@ -249,6 +245,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
> *iter)
> goto Done;
> Fill:
> /* they want more? let's try to get some more */
> + /* m->count is positive and there's space left in iter */
> while (1) {
> size_t offs = m->count;
> loff_t pos = m->index;
> @@ -263,7 +260,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
> *iter)
> err = PTR_ERR(p);
> break;
> }
> - if (m->count >= size)
> + if (m->count >= iov_iter_count(iter))
> break;
> err = m->op->show(m, p);
> if (seq_has_overflowed(m) || err) {
> @@ -273,16 +270,14 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct
> iov_iter *iter)
> }
> }
> m->op->stop(m, p);
> - n = min(m->count, size);
> - if (copy_to_iter(m->buf, n, iter) != n)
> - goto Efault;
> + n = copy_to_iter(m->buf, m->count, iter);
> copied += n;
> m->count -= n;
> m->from = n;
> Done:
> - if (!copied)
> - copied = err;
> - else {
> + if (unlikely(!copied)) {
> + copied = m->count ? -EFAULT : err;
> + } else {
> iocb->ki_pos += copied;
> m->read_pos += copied;
> }
> @@ -291,9 +286,6 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
> *iter)
> Enomem:
> err = -ENOMEM;
> goto Done;
> -Efault:
> - err = -EFAULT;
> - goto Done;
> }
> EXPORT_SYMBOL(seq_read_iter);
>
This patch in -next (6a9f696d1627bacc91d1cebcfb177f474484e8ba) breaks
WSL2's interoperability feature, where Windows paths automatically get
added to PATH on start up so that Windows binaries can be accessed from
within Linux (such as clip.exe to pipe output to the clipboard). Before,
I would see a bunch of Linux + Windows folders in $PATH but after, I
only see the Linux folders (I can give you the actual PATH value if you
care but it is really long).
I am not at all familiar with the semantics of this patch or how
Microsoft would be using it to inject folders into PATH (they have some
documentation on it here:
https://docs.microsoft.com/en-us/windows/wsl/interop) and I am not sure
how to go about figuring that out to see why this patch breaks something
(unless you have an idea). I have added the Hyper-V maintainers and list
to CC in case they know someone who could help.
Cheers,
Nathan
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Wed, Nov 11, 2020 at 2:21 PM Al Viro wrote: > > Something like below (build-tested only): Apart from my usual "oh, Gods, the iter model really does confuse me" this looks more like what I expected, yes. Considering the original bug, I'm clearly not the only one confused by the iov_iter helper functions and the rules.. Linus
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Wed, Nov 11, 2020 at 02:27:02PM -0800, Linus Torvalds wrote: > On Wed, Nov 11, 2020 at 2:21 PM Al Viro wrote: > > > > Something like below (build-tested only): > > Apart from my usual "oh, Gods, the iter model really does confuse me" > this looks more like what I expected, yes. > > Considering the original bug, I'm clearly not the only one confused by > the iov_iter helper functions and the rules.. copy_to_iter() returns the amount it has actually copied, that's all; the cause of that bug is not the primitives used, it's the rules for ->read_iter(). The rules are actually fairly simple - "->read_iter() should not report less data than it has actually left there". For read(2) it's a matter of QoI - if we hit an unmapped page, POSIX pretty much says that all bets are off; read(fd, unmapped - 5, 8) might copy 5 bytes and return 4. It is allowed (and read(2) on those files used to do just that), but it's nicer not to do so. For generic_file_splice_read(), OTOH, it's a bug - we end up with stray data spewed into pipe. So converting to ->read_iter() needs some care. Probably something along those lines should go into D/f/porting...
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Wed, Nov 11, 2020 at 09:52:20PM +, Al Viro wrote:
> That can be done, but I would rather go with
> n = copy_to_iter(m->buf + m->from, m->count, iter);
> m->count -= n;
> m->from += n;
> copied += n;
> if (!size)
> goto Done;
> if (m->count)
> goto Efault;
> if we do it that way. Let me see if I can cook something
> reasonable along those lines...
Something like below (build-tested only):
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3b20e21604e7..07b33c1f34a9 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -168,7 +168,6 @@ EXPORT_SYMBOL(seq_read);
ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
{
struct seq_file *m = iocb->ki_filp->private_data;
- size_t size = iov_iter_count(iter);
size_t copied = 0;
size_t n;
void *p;
@@ -208,14 +207,11 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
*iter)
}
/* if not empty - flush it first */
if (m->count) {
- n = min(m->count, size);
- if (copy_to_iter(m->buf + m->from, n, iter) != n)
- goto Efault;
+ n = copy_to_iter(m->buf + m->from, m->count, iter);
m->count -= n;
m->from += n;
- size -= n;
copied += n;
- if (!size)
+ if (!iov_iter_count(iter) || m->count)
goto Done;
}
/* we need at least one record in buffer */
@@ -249,6 +245,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
*iter)
goto Done;
Fill:
/* they want more? let's try to get some more */
+ /* m->count is positive and there's space left in iter */
while (1) {
size_t offs = m->count;
loff_t pos = m->index;
@@ -263,7 +260,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
*iter)
err = PTR_ERR(p);
break;
}
- if (m->count >= size)
+ if (m->count >= iov_iter_count(iter))
break;
err = m->op->show(m, p);
if (seq_has_overflowed(m) || err) {
@@ -273,16 +270,14 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
*iter)
}
}
m->op->stop(m, p);
- n = min(m->count, size);
- if (copy_to_iter(m->buf, n, iter) != n)
- goto Efault;
+ n = copy_to_iter(m->buf, m->count, iter);
copied += n;
m->count -= n;
m->from = n;
Done:
- if (!copied)
- copied = err;
- else {
+ if (unlikely(!copied)) {
+ copied = m->count ? -EFAULT : err;
+ } else {
iocb->ki_pos += copied;
m->read_pos += copied;
}
@@ -291,9 +286,6 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
*iter)
Enomem:
err = -ENOMEM;
goto Done;
-Efault:
- err = -EFAULT;
- goto Done;
}
EXPORT_SYMBOL(seq_read_iter);
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Wed, Nov 11, 2020 at 09:54:12AM -0800, Linus Torvalds wrote:
> On Tue, Nov 10, 2020 at 3:20 PM Al Viro wrote:
> >
> > Any objections to the following?
>
> Well, I don't _object_, but I find it ugly.
>
> And I think both the old and the "fixed" code is wrong when an EFAULT
> happens in the middle.
>
> Yes, we can just return EFAULT. But for read() and write() we really
> try to do the proper partial returns in other places, why not here?
>
> IOW, why isn't the proper fix just something like this:
>
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 3b20e21604e7..ecc6909b71f5 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -209,7 +209,8 @@ ssize_t seq_read_iter(struct kiocb *iocb,
> struct iov_iter *iter)
> /* if not empty - flush it first */
> if (m->count) {
> n = min(m->count, size);
> - if (copy_to_iter(m->buf + m->from, n, iter) != n)
> + n = copy_to_iter(m->buf + m->from, n, iter);
> + if (!n)
> goto Efault;
> m->count -= n;
> m->from += n;
>
> which should get the "efault in the middle" case roughly right (ie the
> usual "exact byte alignment and page crosser" caveats apply).
Look at the loop after that one, specifically the "it doesn't fit,
allocate a bigger one" part:
kvfree(m->buf);
m->count = 0;
m->buf = seq_buf_alloc(m->size <<= 1);
It really depends upon having m->buf empty at the beginning of
the loop. Your variant will lose the data, unless we copy the
"old" part of buffer (from before the ->show()) into the
larger one.
That can be done, but I would rather go with
n = copy_to_iter(m->buf + m->from, m->count, iter);
m->count -= n;
m->from += n;
copied += n;
if (!size)
goto Done;
if (m->count)
goto Efault;
if we do it that way. Let me see if I can cook something
reasonable along those lines...
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Tue, Nov 10, 2020 at 3:20 PM Al Viro wrote:
>
> Any objections to the following?
Well, I don't _object_, but I find it ugly.
And I think both the old and the "fixed" code is wrong when an EFAULT
happens in the middle.
Yes, we can just return EFAULT. But for read() and write() we really
try to do the proper partial returns in other places, why not here?
IOW, why isn't the proper fix just something like this:
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3b20e21604e7..ecc6909b71f5 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -209,7 +209,8 @@ ssize_t seq_read_iter(struct kiocb *iocb,
struct iov_iter *iter)
/* if not empty - flush it first */
if (m->count) {
n = min(m->count, size);
- if (copy_to_iter(m->buf + m->from, n, iter) != n)
+ n = copy_to_iter(m->buf + m->from, n, iter);
+ if (!n)
goto Efault;
m->count -= n;
m->from += n;
which should get the "efault in the middle" case roughly right (ie the
usual "exact byte alignment and page crosser" caveats apply).
Linus
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Tue, Nov 10, 2020 at 11:20:28PM +, Al Viro wrote: > On Tue, Nov 10, 2020 at 09:35:11PM +, Al Viro wrote: > > On Tue, Nov 10, 2020 at 09:32:53PM +, Al Viro wrote: > > > > > AFAICS, not all callers want that semantics, but I think it's worth > > > a new primitive. I'm not saying it should be a prereq for your > > > series, but either that or an explicit iov_iter_revert() is needed. > > > > Seeing that it already went into mainline, it needs a followup fix. > > And since it's not -stable fodder (AFAICS), I'd rather go with > > adding a new primitive... > > Any objections to the following? > > Fix seq_read_iter() behaviour on full pipe > > generic_file_splice_read() will purge what we'd left in pipe in case > of error; it will *not* do so in case of short write, so we must make > sure that reported amount of data stored by ->read_iter() matches the > reality. > > It's not a rare situation (and we already have it open-coded in at least > one place), so let's introduce a new primitive - copy_to_iter_full(). > Similar to copy_from_iter_full(), it returns true if we had been able > to copy everything we'd been asked to and false otherwise. Iterator > is advanced only on success. > > Signed-off-by: Al Viro Looks ok to me.
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Tue, Nov 10, 2020 at 09:35:11PM +, Al Viro wrote:
> On Tue, Nov 10, 2020 at 09:32:53PM +, Al Viro wrote:
>
> > AFAICS, not all callers want that semantics, but I think it's worth
> > a new primitive. I'm not saying it should be a prereq for your
> > series, but either that or an explicit iov_iter_revert() is needed.
>
> Seeing that it already went into mainline, it needs a followup fix.
> And since it's not -stable fodder (AFAICS), I'd rather go with
> adding a new primitive...
Any objections to the following?
Fix seq_read_iter() behaviour on full pipe
generic_file_splice_read() will purge what we'd left in pipe in case
of error; it will *not* do so in case of short write, so we must make
sure that reported amount of data stored by ->read_iter() matches the
reality.
It's not a rare situation (and we already have it open-coded in at least
one place), so let's introduce a new primitive - copy_to_iter_full().
Similar to copy_from_iter_full(), it returns true if we had been able
to copy everything we'd been asked to and false otherwise. Iterator
is advanced only on success.
Signed-off-by: Al Viro
---
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3b20e21604e7..233d790ea301 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -209,7 +209,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
*iter)
/* if not empty - flush it first */
if (m->count) {
n = min(m->count, size);
- if (copy_to_iter(m->buf + m->from, n, iter) != n)
+ if (!copy_to_iter_full(m->buf + m->from, n, iter))
goto Efault;
m->count -= n;
m->from += n;
@@ -274,7 +274,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
*iter)
}
m->op->stop(m, p);
n = min(m->count, size);
- if (copy_to_iter(m->buf, n, iter) != n)
+ if (!copy_to_iter_full(m->buf, n, iter))
goto Efault;
copied += n;
m->count -= n;
diff --git a/include/linux/uio.h b/include/linux/uio.h
index 72d88566694e..388c05e371ad 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -138,6 +138,18 @@ size_t copy_to_iter(const void *addr, size_t bytes, struct
iov_iter *i)
}
static __always_inline __must_check
+bool copy_to_iter_full(const void *addr, size_t bytes, struct iov_iter *i)
+{
+ if (likely(check_copy_size(addr, bytes, true))) {
+ size_t n = _copy_to_iter(addr, bytes, i);
+ if (likely(n == bytes))
+ return true;
+ iov_iter_revert(i, n);
+ }
+ return false;
+}
+
+static __always_inline __must_check
size_t copy_from_iter(void *addr, size_t bytes, struct iov_iter *i)
{
if (unlikely(!check_copy_size(addr, bytes, false)))
diff --git a/include/net/udp.h b/include/net/udp.h
index 295d52a73598..91d1a2998a2d 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -390,14 +390,7 @@ static inline bool udp_skb_is_linear(struct sk_buff *skb)
static inline int copy_linear_skb(struct sk_buff *skb, int len, int off,
struct iov_iter *to)
{
- int n;
-
- n = copy_to_iter(skb->data + off, len, to);
- if (n == len)
- return 0;
-
- iov_iter_revert(to, n);
- return -EFAULT;
+ return copy_to_iter_full(skb->data + off, len, to) ? 0 : -EFAULT;
}
/*
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Tue, Nov 10, 2020 at 09:32:53PM +, Al Viro wrote: > AFAICS, not all callers want that semantics, but I think it's worth > a new primitive. I'm not saying it should be a prereq for your > series, but either that or an explicit iov_iter_revert() is needed. Seeing that it already went into mainline, it needs a followup fix. And since it's not -stable fodder (AFAICS), I'd rather go with adding a new primitive...
Re: [PATCH 1/6] seq_file: add seq_read_iter
On Wed, Nov 04, 2020 at 09:27:33AM +0100, Christoph Hellwig wrote:
> ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t
> *ppos)
> {
> - struct seq_file *m = file->private_data;
> + struct iovec iov = { .iov_base = buf, .iov_len = size};
> + struct kiocb kiocb;
> + struct iov_iter iter;
> + ssize_t ret;
> +
> + init_sync_kiocb(, file);
> + iov_iter_init(, READ, , 1, size);
> +
> + kiocb.ki_pos = *ppos;
> + ret = seq_read_iter(, );
> + *ppos = kiocb.ki_pos;
> + return ret;
> +}
> +EXPORT_SYMBOL(seq_read);
This is basically an open-coded copy of new_sync_read()...
> if (m->count) {
> n = min(m->count, size);
> - err = copy_to_user(buf, m->buf + m->from, n);
> - if (err)
> + if (copy_to_iter(m->buf + m->from, n, iter) != n)
> goto Efault;
> m->count -= n;
> m->from += n;
> size -= n;
> - buf += n;
> copied += n;
> if (!size)
> goto Done;
> n = min(m->count, size);
> - err = copy_to_user(buf, m->buf, n);
> - if (err)
> + if (copy_to_iter(m->buf, n, iter) != n)
> goto Efault;
This is actually broken from generic_file_splice_read() POV; if you've
already emitted something, you will end up with more data spewed into
pipe than you report to caller. You want something similar to
copy_to_iter_full() here, with iterator _not_ advanced in case of
failure. The first call is not an issue (you have no data copied
yet, so you'll end up with -EFAULT, aka "discard everything you've
put there and return -EAGAIN"), but the second really is a problem.
BTW, other ->read_iter() instances might need to be careful with that
pattern as well; drivers/gpu/drm/drm_dp_aux_dev.c:auxdev_read_iter()
would appear to have the same problem.
if (unlikely(iov_iter_is_pipe(iter))) {
void *addr = kmap_atomic(page);
written = copy_to_iter(addr, copy, iter);
kunmap_atomic(addr);
} else
in fs/cifs/file.c looks... interesting, considering the fact that
copy_to_iter() for pipe destination might very well have to do
allocations. With GFP_USER. Under kmap_atomic()...
Note that we have this:
static inline int copy_linear_skb(struct sk_buff *skb, int len, int off,
struct iov_iter *to)
{
int n;
n = copy_to_iter(skb->data + off, len, to);
if (n == len)
return 0;
iov_iter_revert(to, n);
return -EFAULT;
}
i.e. the same "do not advance on short copy" kind of thing.
AFAICS, not all callers want that semantics, but I think it's worth
a new primitive. I'm not saying it should be a prereq for your
series, but either that or an explicit iov_iter_revert() is needed.
