| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20180711133313.GC835@nautica>
Date: Wed, 11 Jul 2018 15:33:13 +0200
From: Dominique Martinet <asmadeus@...ewreck.org>
To: Matthew Wilcox <willy@...radead.org>
Cc: v9fs-developer@...ts.sourceforge.net,
Latchesar Ionkov <lucho@...kov.net>,
Eric Van Hensbergen <ericvh@...il.com>,
linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
Ron Minnich <rminnich@...dia.gov>
Subject: Re: [V9fs-developer] [PATCH 5/6] 9p: Use a slab for allocating
requests
Matthew Wilcox wrote on Thu, Jun 28, 2018:
> diff --git a/include/net/9p/client.h b/include/net/9p/client.h
> index 0fa0fbab33b0..fd326811ebd4 100644
> --- a/include/net/9p/client.h
> +++ b/include/net/9p/client.h
> @@ -92,24 +85,12 @@ enum p9_req_status_t {
> * struct p9_req_t - request slots
> * @status: status of this request slot
> * @t_err: transport error
> - * @flush_tag: tag of request being flushed (for flush requests)
> * @wq: wait_queue for the client to block on for this request
> * @tc: the request fcall structure
> * @rc: the response fcall structure
> * @aux: transport specific data (provided for trans_fd migration)
> * @req_list: link for higher level objects to chain requests
> - *
> - * Transport use an array to track outstanding requests
> - * instead of a list. While this may incurr overhead during initial
> - * allocation or expansion, it makes request lookup much easier as the
> - * tag id is a index into an array. (We use tag+1 so that we can accommodate
> - * the -1 tag for the T_VERSION request).
> - * This also has the nice effect of only having to allocate wait_queues
> - * once, instead of constantly allocating and freeing them. Its possible
> - * other resources could benefit from this scheme as well.
> - *
> */
> -
> struct p9_req_t {
> int status;
> int t_err;
> @@ -117,40 +98,26 @@ struct p9_req_t {
> struct p9_fcall *tc;
> struct p9_fcall *rc;
> void *aux;
> -
> struct list_head req_list;
> };
>
> /**
> * struct p9_client - per client instance state
> - * @lock: protect @fidlist
> + * @lock: protect @fids and @reqs
> * @msize: maximum data size negotiated by protocol
> - * @dotu: extension flags negotiated by protocol
> * @proto_version: 9P protocol version to use
> * @trans_mod: module API instantiated with this client
> + * @status: XXX
Let's give this a proper comment; something like 'connection state
machine' ? (this contains Connected/BeginDisconnect/Disconnected/Hung)
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 2dce8d8307cc..fd5446377445 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -241,132 +241,104 @@ static struct p9_fcall *p9_fcall_alloc(int alloc_msize)
> return fc;
> }
>
> +static struct kmem_cache *p9_req_cache;
> +
> /**
> - * p9_tag_alloc - lookup/allocate a request by tag
> - * @c: client session to lookup tag within
> - * @tag: numeric id for transaction
> - *
> - * this is a simple array lookup, but will grow the
> - * request_slots as necessary to accommodate transaction
> - * ids which did not previously have a slot.
> - *
> - * this code relies on the client spinlock to manage locks, its
> - * possible we should switch to something else, but I'd rather
> - * stick with something low-overhead for the common case.
> + * p9_req_alloc - Allocate a new request.
> + * @c: Client session.
> + * @type: Transaction type.
> + * @max_size: Maximum packet size for this request.
> *
> + * Context: Process context. What mutex might we be holding that
> + * requires GFP_NOFS?
Good question, but p9_tag_alloc happens on every single client request
so the fs/9p functions might be trying to do something and the alloc
request here comes in and could try to destroy the inode that is
currently used in the request -- I'm not sure how likely this is, but
I'd rather not tempt fate :p
> + * Return: Pointer to new request.
> */
> -
> static struct p9_req_t *
> -p9_tag_alloc(struct p9_client *c, u16 tag, unsigned int max_size)
> +p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size)
> {
> - unsigned long flags;
> - int row, col;
> - struct p9_req_t *req;
> + struct p9_req_t *req = kmem_cache_alloc(p9_req_cache, GFP_NOFS);
> int alloc_msize = min(c->msize, max_size);
> + int tag;
>
> - /* This looks up the original request by tag so we know which
> - * buffer to read the data into */
> - tag++;
> -
> - if (tag >= c->max_tag) {
> - spin_lock_irqsave(&c->lock, flags);
> - /* check again since original check was outside of lock */
> - while (tag >= c->max_tag) {
> - row = (tag / P9_ROW_MAXTAG);
> - c->reqs[row] = kcalloc(P9_ROW_MAXTAG,
> - sizeof(struct p9_req_t), GFP_ATOMIC);
> -
> - if (!c->reqs[row]) {
> - pr_err("Couldn't grow tag array\n");
> - spin_unlock_irqrestore(&c->lock, flags);
> - return ERR_PTR(-ENOMEM);
> - }
> - for (col = 0; col < P9_ROW_MAXTAG; col++) {
> - req = &c->reqs[row][col];
> - req->status = REQ_STATUS_IDLE;
> - init_waitqueue_head(&req->wq);
> - }
> - c->max_tag += P9_ROW_MAXTAG;
> - }
> - spin_unlock_irqrestore(&c->lock, flags);
> - }
> - row = tag / P9_ROW_MAXTAG;
> - col = tag % P9_ROW_MAXTAG;
> + if (!req)
> + return NULL;
>
> - req = &c->reqs[row][col];
> - if (!req->tc)
> - req->tc = p9_fcall_alloc(alloc_msize);
> - if (!req->rc)
> - req->rc = p9_fcall_alloc(alloc_msize);
> + req->tc = p9_fcall_alloc(alloc_msize);
> + req->rc = p9_fcall_alloc(alloc_msize);
> if (!req->tc || !req->rc)
> - goto grow_failed;
> + goto free;
>
> p9pdu_reset(req->tc);
> p9pdu_reset(req->rc);
> -
> - req->tc->tag = tag-1;
> req->status = REQ_STATUS_ALLOC;
> + init_waitqueue_head(&req->wq);
> + INIT_LIST_HEAD(&req->req_list);
> +
> + idr_preload(GFP_NOFS);
> + spin_lock_irq(&c->lock);
> + if (type == P9_TVERSION)
> + tag = idr_alloc(&c->reqs, req, P9_NOTAG, P9_NOTAG + 1,
> + GFP_NOWAIT);
Well this appears to work but P9_NOTAG being '(u16)(~0)' I'm not too
confident with P9_NOTAG + 1. . . it doesn't look like it's overflowing
before the cast on my laptop but is that guaranteed?
> [..]
> @@ -1012,14 +940,11 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)
>
> spin_lock_init(&clnt->lock);
> idr_init(&clnt->fids);
> -
> - err = p9_tag_init(clnt);
> - if (err < 0)
> - goto free_client;
> + idr_init(&clnt->reqs);
I do not see any call to idr_destroy, is that OK?
Looks like another good patch overall, thanks!
--
Dominique
Powered by blists - more mailing lists