Shamelessly copying the explanation from Tetsuo Handa's suggested
patch[1] (slightly reworded):
syzbot is reporting inconsistent lock state in p9_req_put()[2],
for p9_tag_remove() from p9_req_put() from IRQ context is using
spin_lock_irqsave() on "struct p9_client"->lock but trans_fd
(not from IRQ context) is using spin_lock().
Since the locks actually protect different things in client.c and in
trans_fd.c, just replace trans_fd.c's lock by a new one specific to the
transport instead of using spin_lock_irq* variants everywhere
(client.c's protect the idr for tag allocations, while
trans_fd.c's protects its own req list and request status field
that acts as the transport's state machine)
Link: https://lkml.kernel.org/r/[email protected] [1]
Link: https://syzkaller.appspot.com/bug?extid=2f20b523930c32c160cc [2]
Reported-by: syzbot <[email protected]>
Reported-by: Tetsuo Handa <[email protected]>
Signed-off-by: Dominique Martinet <[email protected]>
---
Tetsuo Handa-san, thank you very much!
I'm sorry for not respecting your opinion but it's been a pleasure to
have submissions from someone on JST :)
Both this and your previous patch only impact trans_fd which I can't
test super easily, so while I've sent the patch here I'll only queue it
to -next hopefully next week after I've had time to setup a compatible
server again...
net/9p/trans_fd.c | 42 ++++++++++++++++++++++++++----------------
1 file changed, 26 insertions(+), 16 deletions(-)
diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index ef5760971f1e..5b4807411281 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c
@@ -91,6 +91,7 @@ struct p9_poll_wait {
* @mux_list: list link for mux to manage multiple connections (?)
* @client: reference to client instance for this connection
* @err: error state
+ * @req_lock: lock protecting req_list and requests statuses
* @req_list: accounting for requests which have been sent
* @unsent_req_list: accounting for requests that haven't been sent
* @rreq: read request
@@ -114,6 +115,7 @@ struct p9_conn {
struct list_head mux_list;
struct p9_client *client;
int err;
+ spinlock_t req_lock;
struct list_head req_list;
struct list_head unsent_req_list;
struct p9_req_t *rreq;
@@ -189,10 +191,10 @@ static void p9_conn_cancel(struct p9_conn *m, int err)
p9_debug(P9_DEBUG_ERROR, "mux %p err %d\n", m, err);
- spin_lock(&m->client->lock);
+ spin_lock(&m->req_lock);
if (m->err) {
- spin_unlock(&m->client->lock);
+ spin_unlock(&m->req_lock);
return;
}
@@ -205,7 +207,7 @@ static void p9_conn_cancel(struct p9_conn *m, int err)
list_move(&req->req_list, &cancel_list);
}
- spin_unlock(&m->client->lock);
+ spin_unlock(&m->req_lock);
list_for_each_entry_safe(req, rtmp, &cancel_list, req_list) {
p9_debug(P9_DEBUG_ERROR, "call back req %p\n", req);
@@ -360,7 +362,7 @@ static void p9_read_work(struct work_struct *work)
if ((m->rreq) && (m->rc.offset == m->rc.capacity)) {
p9_debug(P9_DEBUG_TRANS, "got new packet\n");
m->rreq->rc.size = m->rc.offset;
- spin_lock(&m->client->lock);
+ spin_lock(&m->req_lock);
if (m->rreq->status == REQ_STATUS_SENT) {
list_del(&m->rreq->req_list);
p9_client_cb(m->client, m->rreq, REQ_STATUS_RCVD);
@@ -369,14 +371,14 @@ static void p9_read_work(struct work_struct *work)
p9_debug(P9_DEBUG_TRANS,
"Ignore replies associated with a cancelled request\n");
} else {
- spin_unlock(&m->client->lock);
+ spin_unlock(&m->req_lock);
p9_debug(P9_DEBUG_ERROR,
"Request tag %d errored out while we were reading the reply\n",
m->rc.tag);
err = -EIO;
goto error;
}
- spin_unlock(&m->client->lock);
+ spin_unlock(&m->req_lock);
m->rc.sdata = NULL;
m->rc.offset = 0;
m->rc.capacity = 0;
@@ -454,10 +456,10 @@ static void p9_write_work(struct work_struct *work)
}
if (!m->wsize) {
- spin_lock(&m->client->lock);
+ spin_lock(&m->req_lock);
if (list_empty(&m->unsent_req_list)) {
clear_bit(Wworksched, &m->wsched);
- spin_unlock(&m->client->lock);
+ spin_unlock(&m->req_lock);
return;
}
@@ -472,7 +474,7 @@ static void p9_write_work(struct work_struct *work)
m->wpos = 0;
p9_req_get(req);
m->wreq = req;
- spin_unlock(&m->client->lock);
+ spin_unlock(&m->req_lock);
}
p9_debug(P9_DEBUG_TRANS, "mux %p pos %d size %d\n",
@@ -670,10 +672,10 @@ static int p9_fd_request(struct p9_client *client, struct p9_req_t *req)
if (m->err < 0)
return m->err;
- spin_lock(&client->lock);
+ spin_lock(&m->req_lock);
req->status = REQ_STATUS_UNSENT;
list_add_tail(&req->req_list, &m->unsent_req_list);
- spin_unlock(&client->lock);
+ spin_unlock(&m->req_lock);
if (test_and_clear_bit(Wpending, &m->wsched))
n = EPOLLOUT;
@@ -688,11 +690,13 @@ static int p9_fd_request(struct p9_client *client, struct p9_req_t *req)
static int p9_fd_cancel(struct p9_client *client, struct p9_req_t *req)
{
+ struct p9_trans_fd *ts = client->trans;
+ struct p9_conn *m = &ts->conn;
int ret = 1;
p9_debug(P9_DEBUG_TRANS, "client %p req %p\n", client, req);
- spin_lock(&client->lock);
+ spin_lock(&m->req_lock);
if (req->status == REQ_STATUS_UNSENT) {
list_del(&req->req_list);
@@ -700,21 +704,24 @@ static int p9_fd_cancel(struct p9_client *client, struct p9_req_t *req)
p9_req_put(client, req);
ret = 0;
}
- spin_unlock(&client->lock);
+ spin_unlock(&m->req_lock);
return ret;
}
static int p9_fd_cancelled(struct p9_client *client, struct p9_req_t *req)
{
+ struct p9_trans_fd *ts = client->trans;
+ struct p9_conn *m = &ts->conn;
+
p9_debug(P9_DEBUG_TRANS, "client %p req %p\n", client, req);
- spin_lock(&client->lock);
+ spin_lock(&m->req_lock);
/* Ignore cancelled request if message has been received
* before lock.
*/
if (req->status == REQ_STATUS_RCVD) {
- spin_unlock(&client->lock);
+ spin_unlock(&m->req_lock);
return 0;
}
@@ -723,7 +730,8 @@ static int p9_fd_cancelled(struct p9_client *client, struct p9_req_t *req)
*/
list_del(&req->req_list);
req->status = REQ_STATUS_FLSHD;
- spin_unlock(&client->lock);
+ spin_unlock(&m->req_lock);
+
p9_req_put(client, req);
return 0;
@@ -832,6 +840,7 @@ static int p9_fd_open(struct p9_client *client, int rfd, int wfd)
client->trans = ts;
client->status = Connected;
+ spin_lock_init(&ts->conn.req_lock);
return 0;
@@ -866,6 +875,7 @@ static int p9_socket_open(struct p9_client *client, struct socket *csocket)
p->wr = p->rd = file;
client->trans = p;
client->status = Connected;
+ spin_lock_init(&p->conn.req_lock);
p->rd->f_flags |= O_NONBLOCK;
--
2.35.1
On 2022/09/04 20:29, Dominique Martinet wrote:
> Since the locks actually protect different things in client.c and in
> trans_fd.c, just replace trans_fd.c's lock by a new one specific to the
> transport instead of using spin_lock_irq* variants everywhere
> (client.c's protect the idr for tag allocations, while
> trans_fd.c's protects its own req list and request status field
> that acts as the transport's state machine)
OK, I figured out what this patch changes.
$ grep -nF -- '->lock' *.[ch]
client.c:286: spin_lock_irq(&c->lock);
client.c:293: spin_unlock_irq(&c->lock);
client.c:367: spin_lock_irqsave(&c->lock, flags);
client.c:369: spin_unlock_irqrestore(&c->lock, flags);
client.c:816: spin_lock_irq(&clnt->lock);
client.c:819: spin_unlock_irq(&clnt->lock);
client.c:838: spin_lock_irqsave(&clnt->lock, flags);
client.c:840: spin_unlock_irqrestore(&clnt->lock, flags);
client.c:945: spin_lock_init(&clnt->lock);
trans_virtio.c:139: spin_lock_irqsave(&chan->lock, flags);
trans_virtio.c:151: spin_unlock_irqrestore(&chan->lock, flags);
trans_virtio.c:268: spin_lock_irqsave(&chan->lock, flags);
trans_virtio.c:287: spin_unlock_irqrestore(&chan->lock, flags);
trans_virtio.c:296: spin_unlock_irqrestore(&chan->lock, flags);
trans_virtio.c:303: spin_unlock_irqrestore(&chan->lock, flags);
trans_virtio.c:474: spin_lock_irqsave(&chan->lock, flags);
trans_virtio.c:515: spin_unlock_irqrestore(&chan->lock, flags);
trans_virtio.c:524: spin_unlock_irqrestore(&chan->lock, flags);
trans_virtio.c:532: spin_unlock_irqrestore(&chan->lock, flags);
trans_virtio.c:621: spin_lock_init(&chan->lock);
trans_xen.c:142: spin_lock_irqsave(&ring->lock, flags);
trans_xen.c:149: spin_unlock_irqrestore(&ring->lock, flags);
trans_xen.c:164: spin_unlock_irqrestore(&ring->lock, flags);
trans_xen.c:314: spin_lock_init(&ring->lock);
This patch changes "struct p9_client"->lock to be used for only
protecting idr, as explained at
* @lock: protect @fids and @reqs
line. Makes sense and looks correct.
> Tetsuo Handa-san, thank you very much!
> I'm sorry for not respecting your opinion but it's been a pleasure to
> have submissions from someone on JST :)
Thank you for responding quickly. :-)
On Sonntag, 4. September 2022 13:29:28 CEST Dominique Martinet wrote:
> Shamelessly copying the explanation from Tetsuo Handa's suggested
> patch[1] (slightly reworded):
> syzbot is reporting inconsistent lock state in p9_req_put()[2],
> for p9_tag_remove() from p9_req_put() from IRQ context is using
> spin_lock_irqsave() on "struct p9_client"->lock but trans_fd
> (not from IRQ context) is using spin_lock().
>
> Since the locks actually protect different things in client.c and in
> trans_fd.c, just replace trans_fd.c's lock by a new one specific to the
> transport instead of using spin_lock_irq* variants everywhere
> (client.c's protect the idr for tag allocations, while
> trans_fd.c's protects its own req list and request status field
> that acts as the transport's state machine)
>
> Link:
> https://lkml.kernel.org/r/[email protected]
> A.ne.jp [1] Link:
> https://syzkaller.appspot.com/bug?extid=2f20b523930c32c160cc [2]
> Reported-by: syzbot <[email protected]>
> Reported-by: Tetsuo Handa <[email protected]>
> Signed-off-by: Dominique Martinet <[email protected]>
> ---
> Tetsuo Handa-san, thank you very much!
> I'm sorry for not respecting your opinion but it's been a pleasure to
> have submissions from someone on JST :)
>
> Both this and your previous patch only impact trans_fd which I can't
> test super easily, so while I've sent the patch here I'll only queue it
> to -next hopefully next week after I've had time to setup a compatible
> server again...
>
> net/9p/trans_fd.c | 42 ++++++++++++++++++++++++++----------------
> 1 file changed, 26 insertions(+), 16 deletions(-)
>
Late on the party, sorry. Note that you already queued a slightly different
version than this patch here, anyway, one thing ...
> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> index ef5760971f1e..5b4807411281 100644
> --- a/net/9p/trans_fd.c
> +++ b/net/9p/trans_fd.c
> @@ -91,6 +91,7 @@ struct p9_poll_wait {
> * @mux_list: list link for mux to manage multiple connections (?)
> * @client: reference to client instance for this connection
> * @err: error state
> + * @req_lock: lock protecting req_list and requests statuses
> * @req_list: accounting for requests which have been sent
> * @unsent_req_list: accounting for requests that haven't been sent
> * @rreq: read request
> @@ -114,6 +115,7 @@ struct p9_conn {
> struct list_head mux_list;
> struct p9_client *client;
> int err;
> + spinlock_t req_lock;
> struct list_head req_list;
> struct list_head unsent_req_list;
> struct p9_req_t *rreq;
> @@ -189,10 +191,10 @@ static void p9_conn_cancel(struct p9_conn *m, int err)
>
> p9_debug(P9_DEBUG_ERROR, "mux %p err %d\n", m, err);
>
> - spin_lock(&m->client->lock);
> + spin_lock(&m->req_lock);
>
> if (m->err) {
> - spin_unlock(&m->client->lock);
> + spin_unlock(&m->req_lock);
> return;
> }
>
> @@ -205,7 +207,7 @@ static void p9_conn_cancel(struct p9_conn *m, int err)
> list_move(&req->req_list, &cancel_list);
> }
>
> - spin_unlock(&m->client->lock);
> + spin_unlock(&m->req_lock);
>
> list_for_each_entry_safe(req, rtmp, &cancel_list, req_list) {
> p9_debug(P9_DEBUG_ERROR, "call back req %p\n", req);
> @@ -360,7 +362,7 @@ static void p9_read_work(struct work_struct *work)
> if ((m->rreq) && (m->rc.offset == m->rc.capacity)) {
> p9_debug(P9_DEBUG_TRANS, "got new packet\n");
> m->rreq->rc.size = m->rc.offset;
> - spin_lock(&m->client->lock);
> + spin_lock(&m->req_lock);
> if (m->rreq->status == REQ_STATUS_SENT) {
> list_del(&m->rreq->req_list);
> p9_client_cb(m->client, m->rreq,
REQ_STATUS_RCVD);
> @@ -369,14 +371,14 @@ static void p9_read_work(struct work_struct *work)
> p9_debug(P9_DEBUG_TRANS,
> "Ignore replies associated with a
cancelled request\n");
> } else {
> - spin_unlock(&m->client->lock);
> + spin_unlock(&m->req_lock);
> p9_debug(P9_DEBUG_ERROR,
> "Request tag %d errored out while we
were reading the reply\n",
> m->rc.tag);
> err = -EIO;
> goto error;
> }
> - spin_unlock(&m->client->lock);
> + spin_unlock(&m->req_lock);
> m->rc.sdata = NULL;
> m->rc.offset = 0;
> m->rc.capacity = 0;
> @@ -454,10 +456,10 @@ static void p9_write_work(struct work_struct *work)
> }
>
> if (!m->wsize) {
> - spin_lock(&m->client->lock);
> + spin_lock(&m->req_lock);
> if (list_empty(&m->unsent_req_list)) {
> clear_bit(Wworksched, &m->wsched);
> - spin_unlock(&m->client->lock);
> + spin_unlock(&m->req_lock);
> return;
> }
>
> @@ -472,7 +474,7 @@ static void p9_write_work(struct work_struct *work)
> m->wpos = 0;
> p9_req_get(req);
> m->wreq = req;
> - spin_unlock(&m->client->lock);
> + spin_unlock(&m->req_lock);
> }
>
> p9_debug(P9_DEBUG_TRANS, "mux %p pos %d size %d\n",
> @@ -670,10 +672,10 @@ static int p9_fd_request(struct p9_client *client,
> struct p9_req_t *req) if (m->err < 0)
> return m->err;
>
> - spin_lock(&client->lock);
> + spin_lock(&m->req_lock);
> req->status = REQ_STATUS_UNSENT;
> list_add_tail(&req->req_list, &m->unsent_req_list);
> - spin_unlock(&client->lock);
> + spin_unlock(&m->req_lock);
>
> if (test_and_clear_bit(Wpending, &m->wsched))
> n = EPOLLOUT;
> @@ -688,11 +690,13 @@ static int p9_fd_request(struct p9_client *client,
> struct p9_req_t *req)
>
> static int p9_fd_cancel(struct p9_client *client, struct p9_req_t *req)
> {
> + struct p9_trans_fd *ts = client->trans;
> + struct p9_conn *m = &ts->conn;
> int ret = 1;
>
> p9_debug(P9_DEBUG_TRANS, "client %p req %p\n", client, req);
>
> - spin_lock(&client->lock);
> + spin_lock(&m->req_lock);
>
> if (req->status == REQ_STATUS_UNSENT) {
> list_del(&req->req_list);
> @@ -700,21 +704,24 @@ static int p9_fd_cancel(struct p9_client *client,
> struct p9_req_t *req) p9_req_put(client, req);
> ret = 0;
> }
> - spin_unlock(&client->lock);
> + spin_unlock(&m->req_lock);
>
> return ret;
> }
>
> static int p9_fd_cancelled(struct p9_client *client, struct p9_req_t *req)
> {
> + struct p9_trans_fd *ts = client->trans;
> + struct p9_conn *m = &ts->conn;
> +
> p9_debug(P9_DEBUG_TRANS, "client %p req %p\n", client, req);
>
> - spin_lock(&client->lock);
> + spin_lock(&m->req_lock);
> /* Ignore cancelled request if message has been received
> * before lock.
> */
> if (req->status == REQ_STATUS_RCVD) {
> - spin_unlock(&client->lock);
> + spin_unlock(&m->req_lock);
> return 0;
> }
>
> @@ -723,7 +730,8 @@ static int p9_fd_cancelled(struct p9_client *client,
> struct p9_req_t *req) */
> list_del(&req->req_list);
> req->status = REQ_STATUS_FLSHD;
> - spin_unlock(&client->lock);
> + spin_unlock(&m->req_lock);
> +
> p9_req_put(client, req);
>
> return 0;
> @@ -832,6 +840,7 @@ static int p9_fd_open(struct p9_client *client, int rfd,
> int wfd)
>
> client->trans = ts;
> client->status = Connected;
> + spin_lock_init(&ts->conn.req_lock);
Are you sure this is the right place for spin_lock_init()? Not rather in
p9_conn_create()?
> return 0;
>
> @@ -866,6 +875,7 @@ static int p9_socket_open(struct p9_client *client,
> struct socket *csocket) p->wr = p->rd = file;
> client->trans = p;
> client->status = Connected;
> + spin_lock_init(&p->conn.req_lock);
Same here.
>
> p->rd->f_flags |= O_NONBLOCK;
The rest LGTM.
Best regards,
Christian Schoenebeck
Christian Schoenebeck wrote on Thu, Oct 06, 2022 at 03:16:40PM +0200:
> > net/9p/trans_fd.c | 42 ++++++++++++++++++++++++++----------------
> > 1 file changed, 26 insertions(+), 16 deletions(-)
>
> Late on the party, sorry. Note that you already queued a slightly different
> version than this patch here, anyway, one thing ...
Did I? Oh, I apparently reworded the commit message a bit, sorry:
(where HEAD is this patch and 1622... the queued patch)
$ git range-diff HEAD^- 16228c9a4215^-
1: e35fb8546af2 ! 1: 16228c9a4215 net/9p: use a dedicated spinlock for trans_fd
@@ Commit message
Since the locks actually protect different things in client.c and in
trans_fd.c, just replace trans_fd.c's lock by a new one specific to the
- transport instead of using spin_lock_irq* variants everywhere
- (client.c's protect the idr for tag allocations, while
- trans_fd.c's protects its own req list and request status field
+ transport (client.c's protect the idr for fid/tag allocations,
+ while trans_fd.c's protects its own req list and request status field
that acts as the transport's state machine)
- Link: https://lkml.kernel.org/r/[email protected]
+ Link: https://lore.kernel.org/r/[email protected]
Link: https://lkml.kernel.org/r/[email protected] [1]
Link: https://syzkaller.appspot.com/bug?extid=2f20b523930c32c160cc [2]
Reported-by: syzbot <[email protected]>
> > @@ -832,6 +840,7 @@ static int p9_fd_open(struct p9_client *client, int rfd,
> > int wfd)
> >
> > client->trans = ts;
> > client->status = Connected;
> > + spin_lock_init(&ts->conn.req_lock);
>
> Are you sure this is the right place for spin_lock_init()? Not rather in
> p9_conn_create()?
Good point, 'ts->conn' (named... m over there for some reason...) is
mostly initialized in p9_conn_create; it makes much more sense to move
it there despite being slightly further away from the allocation.
It's a bit of a pain to check as the code is spread over many paths (fd,
unix, tcp) but it looks equivalent to me:
- p9_fd_open is only called from p9_fd_create which immediately calls
p9_conn_create
- below p9_socket_open itself immediately calls p9_conn_create
I've moved the init and updated my next branch after very basic check
https://github.com/martinetd/linux/commit/e5cfd99e9ea6c13b3f0134585f269c509247ac0e:
----
diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index 5b4807411281..d407f31bb49d 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c
@@ -591,6 +591,7 @@ static void p9_conn_create(struct p9_client *client)
INIT_LIST_HEAD(&m->mux_list);
m->client = client;
+ spin_lock_init(&m->req_lock);
INIT_LIST_HEAD(&m->req_list);
INIT_LIST_HEAD(&m->unsent_req_list);
INIT_WORK(&m->rq, p9_read_work);
@@ -840,7 +841,6 @@ static int p9_fd_open(struct p9_client *client, int rfd, int wfd)
client->trans = ts;
client->status = Connected;
- spin_lock_init(&ts->conn.req_lock);
return 0;
@@ -875,7 +875,6 @@ static int p9_socket_open(struct p9_client *client, struct socket *csocket)
p->wr = p->rd = file;
client->trans = p;
client->status = Connected;
- spin_lock_init(&p->conn.req_lock);
p->rd->f_flags |= O_NONBLOCK;
----
> The rest LGTM.
Thank you for the look :)
--
Dominique
On Freitag, 7. Oktober 2022 03:05:39 CEST Dominique Martinet wrote:
> Christian Schoenebeck wrote on Thu, Oct 06, 2022 at 03:16:40PM +0200:
> > > net/9p/trans_fd.c | 42 ++++++++++++++++++++++++++----------------
> > > 1 file changed, 26 insertions(+), 16 deletions(-)
> >
> > Late on the party, sorry. Note that you already queued a slightly
> > different
> > version than this patch here, anyway, one thing ...
>
> Did I? Oh, I apparently reworded the commit message a bit, sorry:
>
> (where HEAD is this patch and 1622... the queued patch)
>
> $ git range-diff HEAD^- 16228c9a4215^-
> 1: e35fb8546af2 ! 1: 16228c9a4215 net/9p: use a dedicated spinlock for
> trans_fd @@ Commit message
>
> Since the locks actually protect different things in client.c and
> in trans_fd.c, just replace trans_fd.c's lock by a new one specific to the
> - transport instead of using spin_lock_irq* variants everywhere -
> (client.c's protect the idr for tag allocations, while
> - trans_fd.c's protects its own req list and request status field
> + transport (client.c's protect the idr for fid/tag allocations,
> + while trans_fd.c's protects its own req list and request status
> field that acts as the transport's state machine)
>
> - Link:
> https://lkml.kernel.org/r/[email protected] +
> Link:
> https://lore.kernel.org/r/[email protected]
> Link:
> https://lkml.kernel.org/r/[email protected]
> A.ne.jp [1] Link:
> https://syzkaller.appspot.com/bug?extid=2f20b523930c32c160cc [2]
> Reported-by: syzbot <[email protected]>
No, that's not what I meant, but my bad, it was the following chunk that
didn't apply here:
diff a/net/9p/trans_fd.c b/net/9p/trans_fd.c (rejected hunks)
@@ -205,7 +207,7 @@ static void p9_conn_cancel(struct p9_conn *m, int err)
list_move(&req->req_list, &cancel_list);
}
- spin_unlock(&m->client->lock);
+ spin_unlock(&m->req_lock);
list_for_each_entry_safe(req, rtmp, &cancel_list, req_list) {
p9_debug(P9_DEBUG_ERROR, "call back req %p\n", req);
And that was because this patch was based on:
https://github.com/martinetd/linux/commit/52f1c45dde9136f964d
I usually tag patches depending on another patch not being merged yet (and not
being tied to the same series) like:
Based-on: <message-id>
> > > @@ -832,6 +840,7 @@ static int p9_fd_open(struct p9_client *client, int
> > > rfd, int wfd)
> > >
> > > client->trans = ts;
> > > client->status = Connected;
> > >
> > > + spin_lock_init(&ts->conn.req_lock);
> >
> > Are you sure this is the right place for spin_lock_init()? Not rather in
> > p9_conn_create()?
>
> Good point, 'ts->conn' (named... m over there for some reason...) is
> mostly initialized in p9_conn_create; it makes much more sense to move
> it there despite being slightly further away from the allocation.
>
> It's a bit of a pain to check as the code is spread over many paths (fd,
> unix, tcp) but it looks equivalent to me:
> - p9_fd_open is only called from p9_fd_create which immediately calls
> p9_conn_create
> - below p9_socket_open itself immediately calls p9_conn_create
Yeah, looks pretty much the same, but better to have init code at the same
place. Either or.
> I've moved the init and updated my next branch after very basic check
> https://github.com/martinetd/linux/commit/e5cfd99e9ea6c13b3f0134585f269c5092
> 47ac0e: ----
> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> index 5b4807411281..d407f31bb49d 100644
> --- a/net/9p/trans_fd.c
> +++ b/net/9p/trans_fd.c
> @@ -591,6 +591,7 @@ static void p9_conn_create(struct p9_client *client)
> INIT_LIST_HEAD(&m->mux_list);
> m->client = client;
>
> + spin_lock_init(&m->req_lock);
> INIT_LIST_HEAD(&m->req_list);
> INIT_LIST_HEAD(&m->unsent_req_list);
> INIT_WORK(&m->rq, p9_read_work);
> @@ -840,7 +841,6 @@ static int p9_fd_open(struct p9_client *client, int rfd,
> int wfd)
>
> client->trans = ts;
> client->status = Connected;
> - spin_lock_init(&ts->conn.req_lock);
>
> return 0;
>
> @@ -875,7 +875,6 @@ static int p9_socket_open(struct p9_client *client,
> struct socket *csocket) p->wr = p->rd = file;
> client->trans = p;
> client->status = Connected;
> - spin_lock_init(&p->conn.req_lock);
>
> p->rd->f_flags |= O_NONBLOCK;
>
> ----
>
> > The rest LGTM.
>
> Thank you for the look :)
With that changed, you can add my RB-tag. :)
Thanks!
Best regards,
Christian Schoenebeck