[git,pull] vfs part 2
diff mbox

Message ID 20150621211213.GA18732@ZenIV.linux.org.uk
State New
Headers show

Commit Message

Al Viro June 21, 2015, 9:12 p.m. UTC
On Thu, Apr 23, 2015 at 01:16:15PM +0300, Andrey Ryabinin wrote:
> This change caused following:

> This could happen when p9pdu_readf() changes 'count' to some value > iov_iter_count(from):
> 
> p9_client_write():
> <...>
> 		int count = iov_iter_count(from);
> <...>
> 		*err = p9pdu_readf(req->rc, clnt->proto_version, "d", &count);
> <...>
> 		iov_iter_advance(from, count);

*blink*

That's a bug, all right, but I would love to see how you trigger it.
It would require server to respond to "write that many bytes" with "OK,
<greater number> bytes written".  We certainly need to cope with that
(we can't trust the server to be sane), but if that's what is going on,
you've got a server bug as well.

Could you check if the patch below triggers WARN_ON() in it on your
reproducer?  p9_client_read() has a similar issue as well...

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in

Comments

Linus Torvalds June 21, 2015, 9:16 p.m. UTC | #1
On Sun, Jun 21, 2015 at 2:12 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> +               if (count > rsize) {
> +                       WARN_ON(1);
> +                       count = rsize;
> +               }

So if we'd actually want to merge it with the warning, I'd prefer writing it as

        if (WARN_ON_ONCE(count > rsize))
              count = size;

because it's smaller and cannot spam your logs. WARN_ON_ONCE() will
only _warn_ once, but it always returns the conditional for the
warning, so the above does the right thing.

            Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
Al Viro June 21, 2015, 9:35 p.m. UTC | #2
On Sun, Jun 21, 2015 at 02:16:15PM -0700, Linus Torvalds wrote:
> On Sun, Jun 21, 2015 at 2:12 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > +               if (count > rsize) {
> > +                       WARN_ON(1);
> > +                       count = rsize;
> > +               }
> 
> So if we'd actually want to merge it with the warning, I'd prefer writing it as
> 
>         if (WARN_ON_ONCE(count > rsize))
>               count = size;
> 
> because it's smaller and cannot spam your logs. WARN_ON_ONCE() will
> only _warn_ once, but it always returns the conditional for the
> warning, so the above does the right thing.

Sure, but I would really like to verify that this _is_ what's going on
there.  This is just a "please try that on your reproducer to make sure
that it's not something entirely different", thus the lack of S-o-b,
etc.  For the final variant (and we definitely should cope with BS from
server) we probably want to use p9_debug() instead of WARN_ON/WARN_ON_ONCE.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
Andrey Ryabinin June 22, 2015, 12:02 p.m. UTC | #3
On 06/22/2015 12:12 AM, Al Viro wrote:
> On Thu, Apr 23, 2015 at 01:16:15PM +0300, Andrey Ryabinin wrote:
>> This change caused following:
> 
>> This could happen when p9pdu_readf() changes 'count' to some value > iov_iter_count(from):
>>
>> p9_client_write():
>> <...>
>> 		int count = iov_iter_count(from);
>> <...>
>> 		*err = p9pdu_readf(req->rc, clnt->proto_version, "d", &count);
>> <...>
>> 		iov_iter_advance(from, count);
> 
> *blink*
> 
> That's a bug, all right, but I would love to see how you trigger it.
> It would require server to respond to "write that many bytes" with "OK,
> <greater number> bytes written".  We certainly need to cope with that
> (we can't trust the server to be sane), but if that's what is going on,
> you've got a server bug as well.
> 
> Could you check if the patch below triggers WARN_ON() in it on your
> reproducer?  p9_client_read() has a similar issue as well...
> 

I've tried something like your patch before to check the read side
and I haven't seen anything before and don't see it right now.
Though, this doesn't mean that there is no problem with read.
I mean that trinity hits this on write and may just not hit this on read.


--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in

Patch
diff mbox

diff --git a/net/9p/client.c b/net/9p/client.c
index 6f4c4c8..f99bce7 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -1588,6 +1588,10 @@  p9_client_read(struct p9_fid *fid, u64 offset, struct iov_iter *to, int *err)
 			p9_free_req(clnt, req);
 			break;
 		}
+		if (count > rsize) {
+			WARN_ON(1);
+			count = rsize;
+		}
 
 		if (non_zc) {
 			int n = copy_to_iter(dataptr, count, to);
@@ -1650,6 +1654,10 @@  p9_client_write(struct p9_fid *fid, u64 offset, struct iov_iter *from, int *err)
 		}
 
 		p9_debug(P9_DEBUG_9P, "<<< RWRITE count %d\n", count);
+		if (count > rsize) {
+			WARN_ON(1);
+			count = rsize;
+		}
 
 		p9_free_req(clnt, req);
 		iov_iter_advance(from, count);