From: Arnaud Giersch <arnaud.giersch@free.fr>
To: linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: Trond Myklebust <trond.myklebust@fys.uio.no>
Subject: [PATCH 2.6.35] NFSv4: Don't do idmapper upcalls during XDR decode
Date: Thu, 12 Aug 2010 12:15:34 +0200 [thread overview]
Message-ID: <87vd7gcf21.fsf_-_@free.fr> (raw)
In-Reply-To: <wwed3v2rrai.fsf_-_@powwow.iut-bm.univ-fcomte.fr> (Arnaud Giersch's message of "Mon, 05 Jul 2010 15:28:53 +0200")
From: Arnaud Giersch <arnaud.giersch@free.fr>
Move the name to id mapping out of the XDR decode functions.
Add two new fields to struct nfs_fattr: user_name and group_name, that
are defined iff a name to id mapping should be done.
A new function, nfs_fattr_name_to_id(), is defined to call the
idmapper if needed.
This also fixes the empty core dumps that may be produced on NFSv4
mounts since commit 80e52aced138bb41b045a8595a87510f27d8d8c5.
Signed-off-by: Arnaud Giersch <arnaud.giersch@free.fr>
---
Hi,
The purpose of this patch is to avoid empty core dumps on NFS4 mounts,
as reported in http://article.gmane.org/gmane.linux.kernel/998617, or
http://article.gmane.org/gmane.linux.nfs/33390.
This is my attempt to implement the suggestion made by Trond Myklebust
in http://article.gmane.org/gmane.linux.nfs/33391.
A previous submission (http://article.gmane.org/gmane.linux.nfs/33422)
did not get any feedback. The only difference with this first patch is
the test in nfs_free_fattr(), as the fattr parameter may be NULL.
I am currently running a patched kernel on my workstation, and did not
notice any problem.
Regards,
Arnaud Giersch
fs/nfs/inode.c | 35 ++++++++++++
fs/nfs/nfs4xdr.c | 109 ++++++++++++++-----------------------
include/linux/nfs_fs.h | 4 ++
include/linux/nfs_xdr.h | 7 +++
4 files changed, 88 insertions(+), 67 deletions(-)
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 7d2d6c7..b1f7799 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -234,6 +234,35 @@ nfs_init_locked(struct inode *inode, void *opaque)
return 0;
}
+static void
+nfs_fattr_name_to_id(struct nfs_client *clp, struct nfs_fattr *fattr)
+{
+ if (fattr->user_name) {
+ struct nfs_name *name = fattr->user_name;
+ if (nfs_map_name_to_uid(clp, name->buf, name->len,
+ &fattr->uid) == 0)
+ fattr->valid |= NFS_ATTR_FATTR_OWNER;
+ else
+ dprintk("%s: nfs_map_name_to_uid failed!\n",
+ __func__);
+ kfree(fattr->user_name);
+ fattr->user_name = NULL;
+ dprintk("%s: uid=%d\n", __func__, (int)fattr->uid);
+ }
+ if (fattr->group_name) {
+ struct nfs_name *name = fattr->group_name;
+ if (nfs_map_group_to_gid(clp, name->buf, name->len,
+ &fattr->gid) == 0)
+ fattr->valid |= NFS_ATTR_FATTR_GROUP;
+ else
+ dprintk("%s: nfs_map_group_to_gid failed!\n",
+ __func__);
+ kfree(fattr->group_name);
+ fattr->group_name = NULL;
+ dprintk("%s: gid=%d\n", __func__, (int)fattr->gid);
+ }
+}
+
/* Don't use READDIRPLUS on directories that we believe are too large */
#define NFS_LIMIT_READDIRPLUS (8*PAGE_SIZE)
@@ -263,6 +292,7 @@ nfs_fhget(struct super_block *sb, struct nfs_fh *fh, struct nfs_fattr *fattr)
inode = ERR_PTR(-ENOMEM);
goto out_no_inode;
}
+ nfs_fattr_name_to_id(NFS_SERVER(inode)->nfs_client, fattr);
if (inode->i_state & I_NEW) {
struct nfs_inode *nfsi = NFS_I(inode);
@@ -997,6 +1027,8 @@ unsigned long nfs_inc_attr_generation_counter(void)
void nfs_fattr_init(struct nfs_fattr *fattr)
{
fattr->valid = 0;
+ fattr->user_name = NULL;
+ fattr->group_name = NULL;
fattr->time_start = jiffies;
fattr->gencount = nfs_inc_attr_generation_counter();
}
@@ -1071,6 +1103,7 @@ int nfs_refresh_inode(struct inode *inode, struct nfs_fattr *fattr)
{
int status;
+ nfs_fattr_name_to_id(NFS_SERVER(inode)->nfs_client, fattr);
if ((fattr->valid & NFS_ATTR_FATTR) == 0)
return 0;
spin_lock(&inode->i_lock);
@@ -1110,6 +1143,7 @@ int nfs_post_op_update_inode(struct inode *inode, struct nfs_fattr *fattr)
{
int status;
+ nfs_fattr_name_to_id(NFS_SERVER(inode)->nfs_client, fattr);
spin_lock(&inode->i_lock);
status = nfs_post_op_update_inode_locked(inode, fattr);
spin_unlock(&inode->i_lock);
@@ -1131,6 +1165,7 @@ int nfs_post_op_update_inode_force_wcc(struct inode *inode, struct nfs_fattr *fa
{
int status;
+ nfs_fattr_name_to_id(NFS_SERVER(inode)->nfs_client, fattr);
spin_lock(&inode->i_lock);
/* Don't do a WCC update if these attributes are already stale */
if ((fattr->valid & NFS_ATTR_FATTR) == 0 ||
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 08ef912..c6e7197 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -3271,13 +3271,13 @@ out_overflow:
}
static int decode_attr_owner(struct xdr_stream *xdr, uint32_t *bitmap,
- struct nfs_client *clp, uint32_t *uid, int may_sleep)
+ struct nfs_client *clp, struct nfs_name **owner)
{
uint32_t len;
__be32 *p;
int ret = 0;
- *uid = -2;
+ BUG_ON(*owner);
if (unlikely(bitmap[1] & (FATTR4_WORD1_OWNER - 1U)))
return -EIO;
if (likely(bitmap[1] & FATTR4_WORD1_OWNER)) {
@@ -3288,20 +3288,18 @@ static int decode_attr_owner(struct xdr_stream *xdr, uint32_t *bitmap,
p = xdr_inline_decode(xdr, len);
if (unlikely(!p))
goto out_overflow;
- if (!may_sleep) {
- /* do nothing */
- } else if (len < XDR_MAX_NETOBJ) {
- if (nfs_map_name_to_uid(clp, (char *)p, len, uid) == 0)
- ret = NFS_ATTR_FATTR_OWNER;
- else
- dprintk("%s: nfs_map_name_to_uid failed!\n",
- __func__);
+ if (len < XDR_MAX_NETOBJ) {
+ *owner = kmalloc((sizeof **owner) + len, GFP_NOFS);
+ if (*owner) {
+ (*owner)->len = len;
+ memcpy((*owner)->buf, p, len);
+ } else
+ dprintk("%s: kmalloc failed!\n", __func__);
} else
dprintk("%s: name too long (%u)!\n",
__func__, len);
bitmap[1] &= ~FATTR4_WORD1_OWNER;
}
- dprintk("%s: uid=%d\n", __func__, (int)*uid);
return ret;
out_overflow:
print_overflow_msg(__func__, xdr);
@@ -3309,13 +3307,13 @@ out_overflow:
}
static int decode_attr_group(struct xdr_stream *xdr, uint32_t *bitmap,
- struct nfs_client *clp, uint32_t *gid, int may_sleep)
+ struct nfs_client *clp, struct nfs_name **group)
{
uint32_t len;
__be32 *p;
int ret = 0;
- *gid = -2;
+ BUG_ON(*group);
if (unlikely(bitmap[1] & (FATTR4_WORD1_OWNER_GROUP - 1U)))
return -EIO;
if (likely(bitmap[1] & FATTR4_WORD1_OWNER_GROUP)) {
@@ -3326,20 +3324,18 @@ static int decode_attr_group(struct xdr_stream *xdr, uint32_t *bitmap,
p = xdr_inline_decode(xdr, len);
if (unlikely(!p))
goto out_overflow;
- if (!may_sleep) {
- /* do nothing */
- } else if (len < XDR_MAX_NETOBJ) {
- if (nfs_map_group_to_gid(clp, (char *)p, len, gid) == 0)
- ret = NFS_ATTR_FATTR_GROUP;
- else
- dprintk("%s: nfs_map_group_to_gid failed!\n",
- __func__);
+ if (len < XDR_MAX_NETOBJ) {
+ *group = kmalloc((sizeof **group) + len, GFP_NOFS);
+ if (*group) {
+ (*group)->len = len;
+ memcpy((*group)->buf, p, len);
+ } else
+ dprintk("%s: kmalloc failed!\n", __func__);
} else
dprintk("%s: name too long (%u)!\n",
__func__, len);
bitmap[1] &= ~FATTR4_WORD1_OWNER_GROUP;
}
- dprintk("%s: gid=%d\n", __func__, (int)*gid);
return ret;
out_overflow:
print_overflow_msg(__func__, xdr);
@@ -3745,7 +3741,7 @@ xdr_error:
}
static int decode_getfattr(struct xdr_stream *xdr, struct nfs_fattr *fattr,
- const struct nfs_server *server, int may_sleep)
+ const struct nfs_server *server)
{
__be32 *savep;
uint32_t attrlen,
@@ -3818,13 +3814,13 @@ static int decode_getfattr(struct xdr_stream *xdr, struct nfs_fattr *fattr,
fattr->valid |= status;
status = decode_attr_owner(xdr, bitmap, server->nfs_client,
- &fattr->uid, may_sleep);
+ &fattr->user_name);
if (status < 0)
goto xdr_error;
fattr->valid |= status;
status = decode_attr_group(xdr, bitmap, server->nfs_client,
- &fattr->gid, may_sleep);
+ &fattr->group_name);
if (status < 0)
goto xdr_error;
fattr->valid |= status;
@@ -4757,8 +4753,7 @@ static int nfs4_xdr_dec_open_downgrade(struct rpc_rqst *rqstp, __be32 *p, struct
status = decode_open_downgrade(&xdr, res);
if (status != 0)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -4785,8 +4780,7 @@ static int nfs4_xdr_dec_access(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_ac
status = decode_access(&xdr, res);
if (status != 0)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -4813,8 +4807,7 @@ static int nfs4_xdr_dec_lookup(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_lo
goto out;
if ((status = decode_getfh(&xdr, res->fh)) != 0)
goto out;
- status = decode_getfattr(&xdr, res->fattr, res->server
- ,!RPC_IS_ASYNC(rqstp->rq_task));
+ status = decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -4838,8 +4831,7 @@ static int nfs4_xdr_dec_lookup_root(struct rpc_rqst *rqstp, __be32 *p, struct nf
if ((status = decode_putrootfh(&xdr)) != 0)
goto out;
if ((status = decode_getfh(&xdr, res->fh)) == 0)
- status = decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ status = decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -4864,8 +4856,7 @@ static int nfs4_xdr_dec_remove(struct rpc_rqst *rqstp, __be32 *p, struct nfs_rem
goto out;
if ((status = decode_remove(&xdr, &res->cinfo)) != 0)
goto out;
- decode_getfattr(&xdr, res->dir_attr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->dir_attr, res->server);
out:
return status;
}
@@ -4895,13 +4886,11 @@ static int nfs4_xdr_dec_rename(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_re
if ((status = decode_rename(&xdr, &res->old_cinfo, &res->new_cinfo)) != 0)
goto out;
/* Current FH is target directory */
- if (decode_getfattr(&xdr, res->new_fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task)) != 0)
+ if (decode_getfattr(&xdr, res->new_fattr, res->server) != 0)
goto out;
if ((status = decode_restorefh(&xdr)) != 0)
goto out;
- decode_getfattr(&xdr, res->old_fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->old_fattr, res->server);
out:
return status;
}
@@ -4934,13 +4923,11 @@ static int nfs4_xdr_dec_link(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_link
* Note order: OP_LINK leaves the directory as the current
* filehandle.
*/
- if (decode_getfattr(&xdr, res->dir_attr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task)) != 0)
+ if (decode_getfattr(&xdr, res->dir_attr, res->server) != 0)
goto out;
if ((status = decode_restorefh(&xdr)) != 0)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -4969,13 +4956,11 @@ static int nfs4_xdr_dec_create(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_cr
goto out;
if ((status = decode_getfh(&xdr, res->fh)) != 0)
goto out;
- if (decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task)) != 0)
+ if (decode_getfattr(&xdr, res->fattr, res->server) != 0)
goto out;
if ((status = decode_restorefh(&xdr)) != 0)
goto out;
- decode_getfattr(&xdr, res->dir_fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->dir_fattr, res->server);
out:
return status;
}
@@ -5007,8 +4992,7 @@ static int nfs4_xdr_dec_getattr(struct rpc_rqst *rqstp, __be32 *p, struct nfs4_g
status = decode_putfh(&xdr);
if (status)
goto out;
- status = decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ status = decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -5115,8 +5099,7 @@ static int nfs4_xdr_dec_close(struct rpc_rqst *rqstp, __be32 *p, struct nfs_clos
* an ESTALE error. Shouldn't be a problem,
* though, since fattr->valid will remain unset.
*/
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -5148,13 +5131,11 @@ static int nfs4_xdr_dec_open(struct rpc_rqst *rqstp, __be32 *p, struct nfs_openr
goto out;
if (decode_getfh(&xdr, &res->fh) != 0)
goto out;
- if (decode_getfattr(&xdr, res->f_attr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task)) != 0)
+ if (decode_getfattr(&xdr, res->f_attr, res->server) != 0)
goto out;
if (decode_restorefh(&xdr) != 0)
goto out;
- decode_getfattr(&xdr, res->dir_attr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->dir_attr, res->server);
out:
return status;
}
@@ -5202,8 +5183,7 @@ static int nfs4_xdr_dec_open_noattr(struct rpc_rqst *rqstp, __be32 *p, struct nf
status = decode_open(&xdr, res);
if (status)
goto out;
- decode_getfattr(&xdr, res->f_attr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->f_attr, res->server);
out:
return status;
}
@@ -5230,8 +5210,7 @@ static int nfs4_xdr_dec_setattr(struct rpc_rqst *rqstp, __be32 *p, struct nfs_se
status = decode_setattr(&xdr);
if (status)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -5418,8 +5397,7 @@ static int nfs4_xdr_dec_write(struct rpc_rqst *rqstp, __be32 *p, struct nfs_writ
status = decode_write(&xdr, res);
if (status)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
if (!status)
status = res->count;
out:
@@ -5448,8 +5426,7 @@ static int nfs4_xdr_dec_commit(struct rpc_rqst *rqstp, __be32 *p, struct nfs_wri
status = decode_commit(&xdr, res);
if (status)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -5615,8 +5592,7 @@ static int nfs4_xdr_dec_delegreturn(struct rpc_rqst *rqstp, __be32 *p, struct nf
status = decode_delegreturn(&xdr);
if (status != 0)
goto out;
- decode_getfattr(&xdr, res->fattr, res->server,
- !RPC_IS_ASYNC(rqstp->rq_task));
+ decode_getfattr(&xdr, res->fattr, res->server);
out:
return status;
}
@@ -5644,8 +5620,7 @@ static int nfs4_xdr_dec_fs_locations(struct rpc_rqst *req, __be32 *p,
goto out;
xdr_enter_page(&xdr, PAGE_SIZE);
status = decode_getfattr(&xdr, &res->fs_locations->fattr,
- res->fs_locations->server,
- !RPC_IS_ASYNC(req->rq_task));
+ res->fs_locations->server);
out:
return status;
}
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 508f8cf..6ce7a08 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -369,6 +369,10 @@ extern struct nfs_fattr *nfs_alloc_fattr(void);
static inline void nfs_free_fattr(const struct nfs_fattr *fattr)
{
+ if (!fattr)
+ return;
+ kfree(fattr->user_name);
+ kfree(fattr->group_name);
kfree(fattr);
}
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index fc46192..fc58d5d 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -27,10 +27,17 @@ static inline int nfs_fsid_equal(const struct nfs_fsid *a, const struct nfs_fsid
return a->major == b->major && a->minor == b->minor;
}
+struct nfs_name {
+ size_t len;
+ char buf[];
+};
+
struct nfs_fattr {
unsigned int valid; /* which fields are valid */
umode_t mode;
__u32 nlink;
+ struct nfs_name *user_name;
+ struct nfs_name *group_name;
__u32 uid;
__u32 gid;
dev_t rdev;
next prev parent reply other threads:[~2010-08-12 10:34 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-07-02 8:01 Empty core dumps on NFSv4 mounts Arnaud Giersch
2010-07-02 13:03 ` Trond Myklebust
2010-07-05 13:28 ` [PATCH/RFC 2.6.35-rc4] NFSv4: Don't do idmapper upcalls during XDR decode Arnaud Giersch
2010-08-12 10:15 ` Arnaud Giersch [this message]
2010-09-21 11:17 ` [PATCH resend] " Arnaud Giersch
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=87vd7gcf21.fsf_-_@free.fr \
--to=arnaud.giersch@free.fr \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=trond.myklebust@fys.uio.no \
/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: link
Be 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.