From: Al Viro <viro@ZenIV.linux.org.uk> To: Andrey Ryabinin <a.ryabinin@samsung.com> Cc: Linus Torvalds <torvalds@linux-foundation.org>, LKML <linux-kernel@vger.kernel.org>, linux-fsdevel <linux-fsdevel@vger.kernel.org>, "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>, Eric Van Hensbergen <ericvh@gmail.com>, linux-nfs@vger.kernel.org Subject: Re: running out of tags in 9P (was Re: [git pull] vfs part 2) Date: Thu, 2 Jul 2015 17:43:32 +0100 [thread overview] Message-ID: <20150702164332.GL17109@ZenIV.linux.org.uk> (raw) In-Reply-To: <55952C6D.50805@samsung.com> On Thu, Jul 02, 2015 at 03:19:57PM +0300, Andrey Ryabinin wrote: > Added: > + if (total > count) > + *(char *)0 = 0 > > and never hit this condition. > OK, so it's definitely a mismatched response. > req->tc->tag = tag-1; > + if (WARN_ON(req->status != REQ_STATUS_IDLE)) > + pr_err("req->status: %d\n", req->status); > req->status = REQ_STATUS_ALLOC; > > return req; > [ 150.259076] 9pnet: req->status: 4 IOW, REQ_STATUS_RCVD. Hmm... Stray tag seen by req_done() after we'd already freed the tag in question? That, or it really would have to had wrapped around... Note that req_done() does *not* check anything about the req - not even that p9_tag_lookup() hasn't returned NULL, so a server sending you any response tagged with number well above anything you'd ever sent will reliably oops you. Frankly, the whole thing needs fuzzing from the server side - start throwing crap at the client and see how badly does it get fucked... Folks, it's a network protocol, with userland servers, no less. You *can't* assume them competent and non-malicious... How much traffic does it take to reproduce that fun, BTW? IOW, is attempting to log the sequence of tag {allocation,freeing}/tag of packet being {sent, received} something completely suicidal, or is it more or less feasible? > I didn't get this. c->reqs[row] is always non-NULL as it should be, so this warning > will trigger all the time. ???? row = (tag / P9_ROW_MAXTAG); c->reqs[row] = kcalloc(P9_ROW_MAXTAG, sizeof(struct p9_req_t), GFP_ATOMIC); and you are seeing c->reqs[row] != NULL *BEFORE* that kcalloc()? All the time, no less? Just to make sure we are on the same page - the delta against mainline I would like tested is this: diff --git a/net/9p/client.c b/net/9p/client.c index 6f4c4c8..fa88c9a 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -248,6 +248,9 @@ p9_tag_alloc(struct p9_client *c, u16 tag, unsigned int max_size) /* check again since original check was outside of lock */ while (tag >= c->max_tag) { row = (tag / P9_ROW_MAXTAG); + + WARN_ON_ONCE(c->reqs[row]); // are we about to leak? + c->reqs[row] = kcalloc(P9_ROW_MAXTAG, sizeof(struct p9_req_t), GFP_ATOMIC); @@ -286,6 +289,8 @@ p9_tag_alloc(struct p9_client *c, u16 tag, unsigned int max_size) p9pdu_reset(req->rc); req->tc->tag = tag-1; + if (req->status != REQ_STATUS_IDLE) + pr_err("using tag %d with odd status (%d)", tag, req->status); req->status = REQ_STATUS_ALLOC; return req; @@ -425,6 +430,8 @@ void p9_client_cb(struct p9_client *c, struct p9_req_t *req, int status) * the other thread wakes up will indeed be seen by the waiting side. */ smp_wmb(); + if (req->status == REQ_STATUS_IDLE) + pr_err("late delivery, tag %d already freed", req->tc->tag); req->status = status; wake_up(req->wq); @@ -693,6 +700,10 @@ static struct p9_req_t *p9_client_prepare_req(struct p9_client *c, tag = p9_idpool_get(c->tagpool); if (tag < 0) return ERR_PTR(-ENOMEM); + if (WARN_ON_ONCE(tag != (u16)tag)) { // wrapped around? + p9_idpool_put(tag, c->tagpool); + return ERR_PTR(-ENOMEM); + } } req = p9_tag_alloc(c, tag, req_size); @@ -1647,7 +1658,10 @@ p9_client_write(struct p9_fid *fid, u64 offset, struct iov_iter *from, int *err) if (*err) { trace_9p_protocol_dump(clnt, req->rc); p9_free_req(clnt, req); + break; } + if (rsize < count) + pr_err("mismatched reply [tag = %d]\n", req->tc->tag); p9_debug(P9_DEBUG_9P, "<<< RWRITE count %d\n", count);
WARNING: multiple messages have this Message-ID (diff)
From: Al Viro <viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org> To: Andrey Ryabinin <a.ryabinin-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> Cc: Linus Torvalds <torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>, LKML <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, linux-fsdevel <linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "Aneesh Kumar K.V" <aneesh.kumar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>, Eric Van Hensbergen <ericvh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Subject: Re: running out of tags in 9P (was Re: [git pull] vfs part 2) Date: Thu, 2 Jul 2015 17:43:32 +0100 [thread overview] Message-ID: <20150702164332.GL17109@ZenIV.linux.org.uk> (raw) In-Reply-To: <55952C6D.50805-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> On Thu, Jul 02, 2015 at 03:19:57PM +0300, Andrey Ryabinin wrote: > Added: > + if (total > count) > + *(char *)0 = 0 > > and never hit this condition. > OK, so it's definitely a mismatched response. > req->tc->tag = tag-1; > + if (WARN_ON(req->status != REQ_STATUS_IDLE)) > + pr_err("req->status: %d\n", req->status); > req->status = REQ_STATUS_ALLOC; > > return req; > [ 150.259076] 9pnet: req->status: 4 IOW, REQ_STATUS_RCVD. Hmm... Stray tag seen by req_done() after we'd already freed the tag in question? That, or it really would have to had wrapped around... Note that req_done() does *not* check anything about the req - not even that p9_tag_lookup() hasn't returned NULL, so a server sending you any response tagged with number well above anything you'd ever sent will reliably oops you. Frankly, the whole thing needs fuzzing from the server side - start throwing crap at the client and see how badly does it get fucked... Folks, it's a network protocol, with userland servers, no less. You *can't* assume them competent and non-malicious... How much traffic does it take to reproduce that fun, BTW? IOW, is attempting to log the sequence of tag {allocation,freeing}/tag of packet being {sent, received} something completely suicidal, or is it more or less feasible? > I didn't get this. c->reqs[row] is always non-NULL as it should be, so this warning > will trigger all the time. ???? row = (tag / P9_ROW_MAXTAG); c->reqs[row] = kcalloc(P9_ROW_MAXTAG, sizeof(struct p9_req_t), GFP_ATOMIC); and you are seeing c->reqs[row] != NULL *BEFORE* that kcalloc()? All the time, no less? Just to make sure we are on the same page - the delta against mainline I would like tested is this: diff --git a/net/9p/client.c b/net/9p/client.c index 6f4c4c8..fa88c9a 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -248,6 +248,9 @@ p9_tag_alloc(struct p9_client *c, u16 tag, unsigned int max_size) /* check again since original check was outside of lock */ while (tag >= c->max_tag) { row = (tag / P9_ROW_MAXTAG); + + WARN_ON_ONCE(c->reqs[row]); // are we about to leak? + c->reqs[row] = kcalloc(P9_ROW_MAXTAG, sizeof(struct p9_req_t), GFP_ATOMIC); @@ -286,6 +289,8 @@ p9_tag_alloc(struct p9_client *c, u16 tag, unsigned int max_size) p9pdu_reset(req->rc); req->tc->tag = tag-1; + if (req->status != REQ_STATUS_IDLE) + pr_err("using tag %d with odd status (%d)", tag, req->status); req->status = REQ_STATUS_ALLOC; return req; @@ -425,6 +430,8 @@ void p9_client_cb(struct p9_client *c, struct p9_req_t *req, int status) * the other thread wakes up will indeed be seen by the waiting side. */ smp_wmb(); + if (req->status == REQ_STATUS_IDLE) + pr_err("late delivery, tag %d already freed", req->tc->tag); req->status = status; wake_up(req->wq); @@ -693,6 +700,10 @@ static struct p9_req_t *p9_client_prepare_req(struct p9_client *c, tag = p9_idpool_get(c->tagpool); if (tag < 0) return ERR_PTR(-ENOMEM); + if (WARN_ON_ONCE(tag != (u16)tag)) { // wrapped around? + p9_idpool_put(tag, c->tagpool); + return ERR_PTR(-ENOMEM); + } } req = p9_tag_alloc(c, tag, req_size); @@ -1647,7 +1658,10 @@ p9_client_write(struct p9_fid *fid, u64 offset, struct iov_iter *from, int *err) if (*err) { trace_9p_protocol_dump(clnt, req->rc); p9_free_req(clnt, req); + break; } + if (rsize < count) + pr_err("mismatched reply [tag = %d]\n", req->tc->tag); p9_debug(P9_DEBUG_9P, "<<< RWRITE count %d\n", count); -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2015-07-02 16:43 UTC|newest] Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-04-15 18:14 [git pull] vfs part 2 Al Viro 2015-04-23 10:16 ` Andrey Ryabinin 2015-05-25 8:30 ` Andrey Ryabinin 2015-06-21 21:12 ` Al Viro 2015-06-21 21:12 ` Al Viro 2015-06-21 21:16 ` Linus Torvalds 2015-06-21 21:16 ` Linus Torvalds 2015-06-21 21:35 ` Al Viro 2015-06-21 21:35 ` Al Viro 2015-06-22 12:02 ` Andrey Ryabinin 2015-06-22 12:02 ` Andrey Ryabinin 2015-07-01 6:27 ` Al Viro 2015-07-01 7:50 ` Andrey Ryabinin 2015-07-01 8:27 ` Al Viro 2015-07-01 8:41 ` Andrey Ryabinin 2015-07-01 8:55 ` Al Viro 2015-07-01 11:25 ` Andrey Ryabinin 2015-07-01 18:44 ` Al Viro 2015-07-02 3:20 ` Al Viro 2015-07-02 4:10 ` running out of tags in 9P (was Re: [git pull] vfs part 2) Al Viro 2015-07-02 7:50 ` Andrey Ryabinin 2015-07-02 7:50 ` Andrey Ryabinin 2015-07-02 7:59 ` Al Viro 2015-07-02 7:59 ` Al Viro 2015-07-02 8:19 ` Andrey Ryabinin 2015-07-02 8:25 ` Al Viro 2015-07-02 8:42 ` Al Viro 2015-07-02 8:42 ` Al Viro 2015-07-02 12:19 ` Andrey Ryabinin 2015-07-02 12:19 ` Andrey Ryabinin 2015-07-02 16:43 ` Al Viro [this message] 2015-07-02 16:43 ` Al Viro 2015-07-02 16:49 ` Al Viro 2015-07-02 16:49 ` Al Viro 2015-07-03 8:19 ` Andrey Ryabinin 2015-07-03 8:19 ` Andrey Ryabinin 2015-07-03 9:42 ` Al Viro 2015-07-03 15:00 ` [PATCH] forgetting to cancel request in interrupted zero-copy 9P RPC " Al Viro 2015-07-03 15:00 ` Al Viro 2015-07-03 19:56 ` Andrey Ryabinin 2015-07-03 19:56 ` Andrey Ryabinin 2015-07-02 20:26 ` running out of tags in 9P " Andrey Ryabinin [not found] ` <5594E5EB.4030808@samsung.com> 2015-07-02 7:50 ` Al Viro 2015-07-02 12:00 ` [git pull] vfs part 2 Jeff Layton 2015-07-02 12:07 ` Jeff Layton 2015-07-02 16:45 ` Al Viro 2015-07-02 17:01 ` Jeff Layton 2015-07-02 17:56 ` Dominique Martinet 2015-07-02 18:43 ` Al Viro 2015-07-02 21:00 ` Dominique Martinet 2015-07-02 18:59 ` Jeff Layton 2015-07-02 20:36 ` Andrey Ryabinin 2015-07-02 18:40 ` Al Viro 2015-07-02 19:16 ` Linus Torvalds 2015-07-02 20:44 ` Al Viro
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20150702164332.GL17109@ZenIV.linux.org.uk \ --to=viro@zeniv.linux.org.uk \ --cc=a.ryabinin@samsung.com \ --cc=aneesh.kumar@linux.vnet.ibm.com \ --cc=ericvh@gmail.com \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-nfs@vger.kernel.org \ --cc=torvalds@linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.