[PATCH] 9p: trans_fd, initialize recv fcall properly if not set

That code really should never be called (rc is allocated in
tag_alloc), but if it had been it couldn't have worked...

Signed-off-by: Dominique Martinet <redacted>
---
 net/9p/trans_fd.c | 3 +++
 1 file changed, 3 insertions(+)

To be honest, I think it might be better to just bail out if we get in
this switch (m->req->rc == NULL after p9_tag_lookup) and not try to
allocate more, because if we get there it's likely a race condition and
silently re-allocating will end up in more troubles than trying to
recover is worth.
Thoughts ?

diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index a270dcc..0d9831a 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c

@@ -363,6 +363,9 @@ static void p9_read_work(struct work_struct *work)
 				err = -ENOMEM;
 				goto error;
 			}
+			m->req->rc.capacity = m->client->msize;
+			m->req->rc.sdata = (char*)m->req->rc +
+						sizeof(struct p9_fcall);
 		}
 		m->rc.sdata = (char *)m->req->rc + sizeof(struct p9_fcall);
 		memcpy(m->rc.sdata, m->tmp_buf, m->rc.capacity);

-- 
1.8.3.1

On Thu, Sep 3, 2015 at 4:38 AM, Dominique Martinet
[off-list ref] wrote:

That code really should never be called (rc is allocated in
tag_alloc), but if it had been it couldn't have worked...

Signed-off-by: Dominique Martinet <redacted>
---
 net/9p/trans_fd.c | 3 +++
 1 file changed, 3 insertions(+)

To be honest, I think it might be better to just bail out if we get in
this switch (m->req->rc == NULL after p9_tag_lookup) and not try to
allocate more, because if we get there it's likely a race condition and
silently re-allocating will end up in more troubles than trying to
recover is worth.
Thoughts ?

Hmmm...trying to rattle my brain and remember why I put it in there
back in 2008.
It might have just been over-defensive programming -- or more likely it just
pre-dated all the zero copy infrastructure which pretty much guaranteed we had
an rc allocated and what is there is vestigial.  I'm happy to accept a
patch which
makes this an assert, or perhaps just resets the connection because something
has gone horribly wrong (similar to the ENOMEM path that is there now).

        -eric

Eric Van Hensbergen wrote on Sat, Sep 05, 2015:

On Thu, Sep 3, 2015 at 4:38 AM, Dominique Martinet
[off-list ref] wrote:

quoted

To be honest, I think it might be better to just bail out if we get in
this switch (m->req->rc == NULL after p9_tag_lookup) and not try to
allocate more, because if we get there it's likely a race condition and
silently re-allocating will end up in more troubles than trying to
recover is worth.
Thoughts ?

Hmmm...trying to rattle my brain and remember why I put it in there
back in 2008.
It might have just been over-defensive programming -- or more likely it just
pre-dated all the zero copy infrastructure which pretty much guaranteed we had
an rc allocated and what is there is vestigial.  I'm happy to accept a
patch which
makes this an assert, or perhaps just resets the connection because something
has gone horribly wrong (similar to the ENOMEM path that is there now).

Yeah, it looks like the safety comes from the zero-copy stuff that came
much later.
Let's go with resetting the connection then. Hmm. EIO is a bit too
generic so would be good to avoid that if possible, but can't think of
anything better...


Speaking of zero-copy, I believe it should be fairly straight-forward to
implement for trans_fd now I've actually looked at it, since we do the
payload read after a p9_tag_lookup, would just need m->req to point to a
zc buffer. Write is similar, if there's a zc buffer just send it after
the header.
The cost is a couple more pointers in req and an extra if in both
workers, that seems pretty reasonable.

Well, I'm not using trans_fd much here (and unfortunately zero-copy
isn't possible at all given the transport protocol for RDMA, at least
for recv), but if anyone cares it probably could be done without too
much hassle for the fd workers.

-- 
Dominique

I thought the nature of trans_fd would have prevented any sort of true
zero copy, but I suppose one less is always welcome :)

        -eric


On Sun, Sep 6, 2015 at 1:55 AM, Dominique Martinet
[off-list ref] wrote:

Eric Van Hensbergen wrote on Sat, Sep 05, 2015:

quoted

On Thu, Sep 3, 2015 at 4:38 AM, Dominique Martinet
[off-list ref] wrote:

quoted

To be honest, I think it might be better to just bail out if we get in
this switch (m->req->rc == NULL after p9_tag_lookup) and not try to
allocate more, because if we get there it's likely a race condition and
silently re-allocating will end up in more troubles than trying to
recover is worth.
Thoughts ?

Hmmm...trying to rattle my brain and remember why I put it in there
back in 2008.
It might have just been over-defensive programming -- or more likely it just
pre-dated all the zero copy infrastructure which pretty much guaranteed we had
an rc allocated and what is there is vestigial.  I'm happy to accept a
patch which
makes this an assert, or perhaps just resets the connection because something
has gone horribly wrong (similar to the ENOMEM path that is there now).

Yeah, it looks like the safety comes from the zero-copy stuff that came
much later.
Let's go with resetting the connection then. Hmm. EIO is a bit too
generic so would be good to avoid that if possible, but can't think of
anything better...


Speaking of zero-copy, I believe it should be fairly straight-forward to
implement for trans_fd now I've actually looked at it, since we do the
payload read after a p9_tag_lookup, would just need m->req to point to a
zc buffer. Write is similar, if there's a zc buffer just send it after
the header.
The cost is a couple more pointers in req and an extra if in both
workers, that seems pretty reasonable.

Well, I'm not using trans_fd much here (and unfortunately zero-copy
isn't possible at all given the transport protocol for RDMA, at least
for recv), but if anyone cares it probably could be done without too
much hassle for the fd workers.

2 more quoted lines

req->rc is pre-allocated early on with p9_tag_alloc and shouldn't be missing

Signed-off-by: Dominique Martinet <redacted>
---
 net/9p/trans_fd.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Feel free to adapt error code/message if you can think of something better.

diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index a270dcc..a6d89c0 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c

@@ -356,13 +356,12 @@ static void p9_read_work(struct work_struct *work)
 		}
 
 		if (m->req->rc == NULL) {
-			m->req->rc = kmalloc(sizeof(struct p9_fcall) +
-						m->client->msize, GFP_NOFS);
-			if (!m->req->rc) {
-				m->req = NULL;
-				err = -ENOMEM;
-				goto error;
-			}
+			p9_debug(P9_DEBUG_ERROR,
+				 "No recv fcall for tag %d (req %p), disconnecting!\n",
+				 m->rc.tag, m->req);
+			m->req = NULL;
+			err = -EIO;
+			goto error;
 		}
 		m->rc.sdata = (char *)m->req->rc + sizeof(struct p9_fcall);
 		memcpy(m->rc.sdata, m->tmp_buf, m->rc.capacity);

-- 
1.8.3.1