On 05/16/2016 02:35 PM, Dilger, Andreas wrote:
On 2016/05/16, 12:16, "James Simmons" <jsimmons@xxxxxxxxxxxxx> wrote:
This looks wrong - You return -EINVAL from sptlrpc_pack_user_desc, but then
the caller checks "!desc". Desc will not be null, since you've returned
-EINVAL.
Actually 'if (!desc)' is equal to 'if (desc != 0). Yes it can be confusing.
Very confusing indeed. :-) "if (!desc)" actually means "if (desc == 0)"...
That is the main reason why I don't like those shortcuts since they require
an extra mental step by the reader to determine the logic. Having the
explicit comparisons like "if (desc == 0)" or "if (desc != 0)" makes the
code more clear, and doesn't make the compiled binary any slower.
I recommend 'if (desc < 0)' instead to make it clearer what is being
tested for.
That would actually fix the problem. Patrick is correct that the current
patch is broken, since it is either returning zero "if (!desc)" is true,
or zero at the end of the function. The error is never returned.
Lidza,
to improve this patch further, the function should really use "rc" to hold
the error return instead of "desc", since "rc" is typically used for error
returns, and "desc" is normally a pointer to a bulk descriptor in this code.
Also, as Oleg previously mentioned, please declare "int rc;" inside the
"if (req->rq_pack_udesc)" block instead of at the top of the function,
since it isn't used anywhere else.
Cheers, Andreas
Definitely, will do.
I'll change desc to rc and update the if statement and send another
patch now.
Lidza
- Patrick
On 05/16/2016 09:17 AM, Lidza Louina wrote:
The lustre_msg_buf method could return NULL. Subsequent code didn't
check if it's null before using it. This patch adds two checks.
Signed-off-by: Lidza Louina <lidza.louina@xxxxxxxxxx>
---
drivers/staging/lustre/lustre/ptlrpc/sec.c | 2 ++
drivers/staging/lustre/lustre/ptlrpc/sec_plain.c | 9 +++++++--
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/lustre/lustre/ptlrpc/sec.c
b/drivers/staging/lustre/lustre/ptlrpc/sec.c
index 187fd1d..e6fedc3 100644
--- a/drivers/staging/lustre/lustre/ptlrpc/sec.c
+++ b/drivers/staging/lustre/lustre/ptlrpc/sec.c
@@ -2195,6 +2195,8 @@ int sptlrpc_pack_user_desc(struct lustre_msg *msg, int
offset)
struct ptlrpc_user_desc *pud;
pud = lustre_msg_buf(msg, offset, 0);
+ if (!pud)
+ return -EINVAL;
pud->pud_uid = from_kuid(&init_user_ns, current_uid());
pud->pud_gid = from_kgid(&init_user_ns, current_gid());
diff --git a/drivers/staging/lustre/lustre/ptlrpc/sec_plain.c
b/drivers/staging/lustre/lustre/ptlrpc/sec_plain.c
index 37c9f4c..51741c8 100644
--- a/drivers/staging/lustre/lustre/ptlrpc/sec_plain.c
+++ b/drivers/staging/lustre/lustre/ptlrpc/sec_plain.c
@@ -542,6 +542,7 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec,
{
__u32 buflens[PLAIN_PACK_SEGMENTS] = { 0, };
int alloc_len;
+ int desc;
buflens[PLAIN_PACK_HDR_OFF] = sizeof(struct plain_header);
buflens[PLAIN_PACK_MSG_OFF] = msgsize;
@@ -574,8 +575,12 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec,
lustre_init_msg_v2(req->rq_reqbuf, PLAIN_PACK_SEGMENTS, buflens, NULL);
req->rq_reqmsg = lustre_msg_buf(req->rq_reqbuf, PLAIN_PACK_MSG_OFF, 0);
- if (req->rq_pack_udesc)
- sptlrpc_pack_user_desc(req->rq_reqbuf, PLAIN_PACK_USER_OFF);
+ if (req->rq_pack_udesc) {
+ desc = sptlrpc_pack_user_desc(req->rq_reqbuf,
+ PLAIN_PACK_USER_OFF);
+ if (!desc)
+ return desc;
+ }
return 0;
}
_______________________________________________
lustre-devel mailing list
lustre-devel@xxxxxxxxxxxxxxxx
http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
Cheers, Andreas
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
