From: Bryan Schumaker <[email protected]>
idmap_pipe_downcall already clears this field if the upcall succeeds,
but if it fails (rpc.idmapd isn't running) the field will still be set
on the next call triggering a BUG_ON(). This patch tries to handle all
possible ways that the upcall could fail and clear the idmap key data
for each one.
Signed-off-by: Bryan Schumaker <[email protected]>
---
fs/nfs/idmap.c | 29 ++++++++++++++++++++++++++---
include/linux/nfs_idmap.h | 1 +
2 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c
index 66d0e85..c2b4004 100644
--- a/fs/nfs/idmap.c
+++ b/fs/nfs/idmap.c
@@ -324,6 +324,7 @@ static ssize_t nfs_idmap_get_key(const char *name, size_t namelen,
ret = nfs_idmap_request_key(&key_type_id_resolver_legacy,
name, namelen, type, data,
data_size, idmap);
+ idmap->idmap_key_cons = NULL;
mutex_unlock(&idmap->idmap_mutex);
}
return ret;
@@ -380,11 +381,13 @@ static const match_table_t nfs_idmap_tokens = {
static int nfs_idmap_legacy_upcall(struct key_construction *, const char *, void *);
static ssize_t idmap_pipe_downcall(struct file *, const char __user *,
size_t);
+static void idmap_release_pipe(struct inode *);
static void idmap_pipe_destroy_msg(struct rpc_pipe_msg *);
static const struct rpc_pipe_ops idmap_upcall_ops = {
.upcall = rpc_pipe_generic_upcall,
.downcall = idmap_pipe_downcall,
+ .release_pipe = idmap_release_pipe,
.destroy_msg = idmap_pipe_destroy_msg,
};
@@ -616,7 +619,8 @@ void nfs_idmap_quit(void)
nfs_idmap_quit_keyring();
}
-static int nfs_idmap_prepare_message(char *desc, struct idmap_msg *im,
+static int nfs_idmap_prepare_message(char *desc, struct idmap *idmap,
+ struct idmap_msg *im,
struct rpc_pipe_msg *msg)
{
substring_t substr;
@@ -626,6 +630,7 @@ static int nfs_idmap_prepare_message(char *desc, struct idmap_msg *im,
memset(msg, 0, sizeof(*msg));
im->im_type = IDMAP_TYPE_GROUP;
+ im->im_private = idmap;
token = match_token(desc, nfs_idmap_tokens, &substr);
switch (token) {
@@ -674,7 +679,7 @@ static int nfs_idmap_legacy_upcall(struct key_construction *cons,
if (!im)
goto out1;
- ret = nfs_idmap_prepare_message(key->description, im, msg);
+ ret = nfs_idmap_prepare_message(key->description, idmap, im, msg);
if (ret < 0)
goto out2;
@@ -683,10 +688,12 @@ static int nfs_idmap_legacy_upcall(struct key_construction *cons,
ret = rpc_queue_upcall(idmap->idmap_pipe, msg);
if (ret < 0)
- goto out2;
+ goto out3;
return ret;
+out3:
+ idmap->idmap_key_cons = NULL;
out2:
kfree(im);
out1:
@@ -775,11 +782,27 @@ out_incomplete:
static void
idmap_pipe_destroy_msg(struct rpc_pipe_msg *msg)
{
+ struct idmap_msg *im = msg->data;
+ struct idmap *idmap = (struct idmap *)im->im_private;
+ struct key_construction *cons;
+ if (msg->errno) {
+ cons = ACCESS_ONCE(idmap->idmap_key_cons);
+ idmap->idmap_key_cons = NULL;
+ complete_request_key(cons, msg->errno);
+ }
/* Free memory allocated in nfs_idmap_legacy_upcall() */
kfree(msg->data);
kfree(msg);
}
+static void
+idmap_release_pipe(struct inode *inode)
+{
+ struct rpc_inode *rpci = RPC_I(inode);
+ struct idmap *idmap = (struct idmap *)rpci->private;
+ idmap->idmap_key_cons = NULL;
+}
+
int nfs_map_name_to_uid(const struct nfs_server *server, const char *name, size_t namelen, __u32 *uid)
{
struct idmap *idmap = server->nfs_client->cl_idmap;
diff --git a/include/linux/nfs_idmap.h b/include/linux/nfs_idmap.h
index ece91c5..8a645c7 100644
--- a/include/linux/nfs_idmap.h
+++ b/include/linux/nfs_idmap.h
@@ -59,6 +59,7 @@ struct idmap_msg {
char im_name[IDMAP_NAMESZ];
__u32 im_id;
__u8 im_status;
+ void *im_private;
};
#ifdef __KERNEL__
--
1.7.11.4
On Thu, Aug 9, 2012 at 8:05 PM, <[email protected]> wrote:
> From: Bryan Schumaker <[email protected]>
>
> This allows the normal error-paths to handle the error, rather than
> making a special call to complete_request_key() just for this instance.
>
> Signed-off-by: Bryan Schumaker <[email protected]>
> ---
> fs/nfs/idmap.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c
> index c2b4004..9864b48 100644
> --- a/fs/nfs/idmap.c
> +++ b/fs/nfs/idmap.c
> @@ -756,9 +756,8 @@ idmap_pipe_downcall(struct file *filp, const char __user *src, size_t mlen)
> }
>
> if (!(im.im_status & IDMAP_STATUS_SUCCESS)) {
> - ret = mlen;
> - complete_request_key(cons, -ENOKEY);
> - goto out_incomplete;
> + ret = -ENOKEY;
> + goto out;
and I think this patch could fix the "unable to handle kernel NULL
pointer dereference on wait_for_key_construction" I reported earlier.
--
William
Hi Bryan,
On Fri, Aug 10, 2012 at 12:09 PM, William Dauchy <[email protected]> wrote:
> strange, I was also getting the BUG_ON even with an idmap running.
On another setup, this patch is breaking the legacy mapping.
rpc.idmapd is running but it can't map any user. I'm trying to see the
difference with the previous test I made.
--
William
On Thu, Aug 9, 2012 at 8:05 PM, <[email protected]> wrote:
> This will allocate memory that has already been zeroed, allowing us to
> remove the memset later on.
>
> Signed-off-by: Bryan Schumaker <[email protected]>
Tested-by: William Dauchy <[email protected]>
Cc: [email protected]
--
William
On Thu, Aug 16, 2012 at 3:45 PM, William Dauchy <[email protected]> wrote:
> On another setup, this patch is breaking the legacy mapping.
> rpc.idmapd is running but it can't map any user. I'm trying to see the
> difference with the previous test I made.
my second test is on 64 bits (the previous was on 32 bits).
I'm also getting many errors from the userland:
rpc.idmapd: nfscb: write(/var/lib/nfs/rpc_pipefs/nfs/clnt4/idmap): No
space left on device
I assume this is because of adding `im_private` in `struct idmap_msg`
since removing the field resolves the issue.
I tried to find a wrong sizeof regarding `struct idmap_msg` but didn?t
found any thing at the moment.
--
William
On Thu, Aug 16, 2012 at 5:21 PM, Bryan Schumaker <[email protected]> wrote:
> I was afraid im_private would cause problems, but I wouldn't expect "No space left on device". You did double check the output of `df`, right? :).
:)
yes.
> I'll play around with it soon to see what I can find. Thanks for finding this!
Thanks. I'm also testing some stuff to find a possible solution.
--
William
On Fri, Aug 10, 2012 at 6:16 PM, Myklebust, Trond
<[email protected]> wrote:
> No. This is a cleanup and so is not stable kernel material.
true; sorry for the glitch
--
William
From: Bryan Schumaker <[email protected]>
This will allocate memory that has already been zeroed, allowing us to
remove the memset later on.
Signed-off-by: Bryan Schumaker <[email protected]>
---
fs/nfs/idmap.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c
index 9864b48..4af524e 100644
--- a/fs/nfs/idmap.c
+++ b/fs/nfs/idmap.c
@@ -626,9 +626,6 @@ static int nfs_idmap_prepare_message(char *desc, struct idmap *idmap,
substring_t substr;
int token, ret;
- memset(im, 0, sizeof(*im));
- memset(msg, 0, sizeof(*msg));
-
im->im_type = IDMAP_TYPE_GROUP;
im->im_private = idmap;
token = match_token(desc, nfs_idmap_tokens, &substr);
@@ -671,11 +668,11 @@ static int nfs_idmap_legacy_upcall(struct key_construction *cons,
int ret = -ENOMEM;
/* msg and im are freed in idmap_pipe_destroy_msg */
- msg = kmalloc(sizeof(*msg), GFP_KERNEL);
+ msg = kzalloc(sizeof(*msg), GFP_KERNEL);
if (!msg)
goto out0;
- im = kmalloc(sizeof(*im), GFP_KERNEL);
+ im = kzalloc(sizeof(*im), GFP_KERNEL);
if (!im)
goto out1;
--
1.7.11.4
T24gRnJpLCAyMDEyLTA4LTEwIGF0IDEyOjEwICswMjAwLCBXaWxsaWFtIERhdWNoeSB3cm90ZToN
Cj4gT24gVGh1LCBBdWcgOSwgMjAxMiBhdCA4OjA1IFBNLCAgPGJqc2NodW1hQG5ldGFwcC5jb20+
IHdyb3RlOg0KPiA+IFRoaXMgd2lsbCBhbGxvY2F0ZSBtZW1vcnkgdGhhdCBoYXMgYWxyZWFkeSBi
ZWVuIHplcm9lZCwgYWxsb3dpbmcgdXMgdG8NCj4gPiByZW1vdmUgdGhlIG1lbXNldCBsYXRlciBv
bi4NCj4gPg0KPiA+IFNpZ25lZC1vZmYtYnk6IEJyeWFuIFNjaHVtYWtlciA8YmpjaHVtYUBuZXRh
cHAuY29tPg0KPiANCj4gVGVzdGVkLWJ5OiBXaWxsaWFtIERhdWNoeSA8d2RhdWNoeUBnbWFpbC5j
b20+DQo+IENjOiBzdGFibGVAdmdlci5rZXJuZWwub3JnDQo+IA0KDQpOby4gVGhpcyBpcyBhIGNs
ZWFudXAgYW5kIHNvIGlzIG5vdCBzdGFibGUga2VybmVsIG1hdGVyaWFsLg0KDQotLSANClRyb25k
IE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQu
TXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQoNCg==
On Thu, Aug 9, 2012 at 8:05 PM, <[email protected]> wrote:
> idmap_pipe_downcall already clears this field if the upcall succeeds,
> but if it fails (rpc.idmapd isn't running) the field will still be set
> on the next call triggering a BUG_ON(). This patch tries to handle all
> possible ways that the upcall could fail and clear the idmap key data
> for each one.
strange, I was also getting the BUG_ON even with an idmap running.
I tested this patch on top of a 3.4.8 kernel; before the patch, I was
unable to mount a nfsv4 mount correctly. I think this should also go
in stable.
> Signed-off-by: Bryan Schumaker <[email protected]>
Tested-by: William Dauchy <[email protected]>
Cc: [email protected]
--
William
On Thu, Aug 9, 2012 at 8:05 PM, <[email protected]> wrote:
> This allows the normal error-paths to handle the error, rather than
> making a special call to complete_request_key() just for this instance.
>
> Signed-off-by: Bryan Schumaker <[email protected]>
Tested-by: William Dauchy <[email protected]>
Cc: [email protected]
--
William
On 08/16/2012 11:07 AM, William Dauchy wrote:
> On Thu, Aug 16, 2012 at 3:45 PM, William Dauchy <[email protected]> wrote:
>> On another setup, this patch is breaking the legacy mapping.
>> rpc.idmapd is running but it can't map any user. I'm trying to see the
>> difference with the previous test I made.
>
> my second test is on 64 bits (the previous was on 32 bits).
> I'm also getting many errors from the userland:
> rpc.idmapd: nfscb: write(/var/lib/nfs/rpc_pipefs/nfs/clnt4/idmap): No
> space left on device
>
> I assume this is because of adding `im_private` in `struct idmap_msg`
> since removing the field resolves the issue.
> I tried to find a wrong sizeof regarding `struct idmap_msg` but didn?t
> found any thing at the moment.
>
I was afraid im_private would cause problems, but I wouldn't expect "No space left on device". You did double check the output of `df`, right? :).
I'll play around with it soon to see what I can find. Thanks for finding this!
- Bryan
From: Bryan Schumaker <[email protected]>
This allows the normal error-paths to handle the error, rather than
making a special call to complete_request_key() just for this instance.
Signed-off-by: Bryan Schumaker <[email protected]>
---
fs/nfs/idmap.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c
index c2b4004..9864b48 100644
--- a/fs/nfs/idmap.c
+++ b/fs/nfs/idmap.c
@@ -756,9 +756,8 @@ idmap_pipe_downcall(struct file *filp, const char __user *src, size_t mlen)
}
if (!(im.im_status & IDMAP_STATUS_SUCCESS)) {
- ret = mlen;
- complete_request_key(cons, -ENOKEY);
- goto out_incomplete;
+ ret = -ENOKEY;
+ goto out;
}
namelen_in = strnlen(im.im_name, IDMAP_NAMESZ);
@@ -775,7 +774,6 @@ idmap_pipe_downcall(struct file *filp, const char __user *src, size_t mlen)
out:
complete_request_key(cons, ret);
-out_incomplete:
return ret;
}
--
1.7.11.4