* [lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference @ 2016-05-16 14:17 Lidza Louina 2016-05-16 17:57 ` Patrick Farrell 0 siblings, 1 reply; 10+ messages in thread From: Lidza Louina @ 2016-05-16 14:17 UTC (permalink / raw) To: lustre-devel 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@oracle.com> --- 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; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference 2016-05-16 14:17 [lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference Lidza Louina @ 2016-05-16 17:57 ` Patrick Farrell 2016-05-16 18:16 ` James Simmons 0 siblings, 1 reply; 10+ messages in thread From: Patrick Farrell @ 2016-05-16 17:57 UTC (permalink / raw) To: lustre-devel 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. - 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@oracle.com> > --- > 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; > } ^ permalink raw reply [flat|nested] 10+ messages in thread
* [lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference 2016-05-16 17:57 ` Patrick Farrell @ 2016-05-16 18:16 ` James Simmons 2016-05-16 18:22 ` Patrick Farrell 2016-05-16 18:35 ` Dilger, Andreas 0 siblings, 2 replies; 10+ messages in thread From: James Simmons @ 2016-05-16 18:16 UTC (permalink / raw) To: lustre-devel > 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. I recommend 'if (desc < 0)' instead to make it clearer what is being tested for. > - 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@oracle.com> > > --- > > 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 at lists.lustre.org > http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference 2016-05-16 18:16 ` James Simmons @ 2016-05-16 18:22 ` Patrick Farrell 2016-05-16 18:25 ` James Simmons 2016-05-16 18:35 ` Dilger, Andreas 1 sibling, 1 reply; 10+ messages in thread From: Patrick Farrell @ 2016-05-16 18:22 UTC (permalink / raw) To: lustre-devel James, No. You've got it backwards. 0 is false, any non-zero value is true. if(desc) would be equal to if (desc != 0). - Patrick On 05/16/2016 01:16 PM, James Simmons 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. > I recommend 'if (desc < 0)' instead to make it clearer what is being > tested for. > >> - 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@oracle.com> >>> --- >>> 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 at lists.lustre.org >> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference 2016-05-16 18:22 ` Patrick Farrell @ 2016-05-16 18:25 ` James Simmons 0 siblings, 0 replies; 10+ messages in thread From: James Simmons @ 2016-05-16 18:25 UTC (permalink / raw) To: lustre-devel > James, > > No. You've got it backwards. 0 is false, any non-zero value is true. > > if(desc) would be equal to if (desc != 0). Ah, you are right. Didn't get much sleep last night :-( > - Patrick > > On 05/16/2016 01:16 PM, James Simmons 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. > > I recommend 'if (desc < 0)' instead to make it clearer what is being > > tested for. > > > > > - 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@oracle.com> > > > > --- > > > > 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 at lists.lustre.org > > > http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference 2016-05-16 18:16 ` James Simmons 2016-05-16 18:22 ` Patrick Farrell @ 2016-05-16 18:35 ` Dilger, Andreas 2016-05-16 18:47 ` Lidza Louina 2016-05-17 6:53 ` Dan Carpenter 1 sibling, 2 replies; 10+ messages in thread From: Dilger, Andreas @ 2016-05-16 18:35 UTC (permalink / raw) To: lustre-devel On 2016/05/16, 12:16, "James Simmons" <jsimmons@infradead.org> 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 >> - 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@oracle.com> >> > --- >> > 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 at lists.lustre.org >> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org >> > Cheers, Andreas -- Andreas Dilger Lustre Principal Architect Intel High Performance Data Division ^ permalink raw reply [flat|nested] 10+ messages in thread
* [lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference 2016-05-16 18:35 ` Dilger, Andreas @ 2016-05-16 18:47 ` Lidza Louina 2016-05-17 6:53 ` Dan Carpenter 1 sibling, 0 replies; 10+ messages in thread From: Lidza Louina @ 2016-05-16 18:47 UTC (permalink / raw) To: lustre-devel On 05/16/2016 02:35 PM, Dilger, Andreas wrote: > On 2016/05/16, 12:16, "James Simmons" <jsimmons@infradead.org> 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@oracle.com> >>>> --- >>>> 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 at lists.lustre.org >>> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org >>> > > Cheers, Andreas ^ permalink raw reply [flat|nested] 10+ messages in thread
* [lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference 2016-05-16 18:35 ` Dilger, Andreas 2016-05-16 18:47 ` Lidza Louina @ 2016-05-17 6:53 ` Dan Carpenter 2016-05-17 14:22 ` Lidza Louina 1 sibling, 1 reply; 10+ messages in thread From: Dan Carpenter @ 2016-05-17 6:53 UTC (permalink / raw) To: lustre-devel When I read the code, I just assumed desc was a pointer and it should have been: if (!desc) return NULL; For me, "if (rc) " is way more readable than "if (rc != 0) ". So readability could go either way depending on what you're used to, I suppose. It should definitely == 0 and != 0 if you are talking about the actual number zero instead of success/fail like we are here. Also it helps to use == 0 with strcmp() and friends (although half of the kernel does not know that trick yet). The other thing which I have noticed recently is that a lot of subsystems use a mix of "if (rc) " and "if (rc < 0) ". It's annoying for Smatch because say a function only returns zero but the some of the callers check for < 0 and some check for != 0. We can't know for sure that they are equivalent. regards, dan carpenter ^ permalink raw reply [flat|nested] 10+ messages in thread
* [lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference 2016-05-17 6:53 ` Dan Carpenter @ 2016-05-17 14:22 ` Lidza Louina 2016-05-17 16:39 ` Dan Carpenter 0 siblings, 1 reply; 10+ messages in thread From: Lidza Louina @ 2016-05-17 14:22 UTC (permalink / raw) To: lustre-devel On 05/17/2016 02:53 AM, Dan Carpenter wrote: > When I read the code, I just assumed desc was a pointer and it should > have been: > > if (!desc) > return NULL; > > For me, "if (rc) " is way more readable than "if (rc != 0) ". So > readability could go either way depending on what you're used to, I > suppose. > > It should definitely == 0 and != 0 if you are talking about the actual > number zero instead of success/fail like we are here. Also it helps to > use == 0 with strcmp() and friends (although half of the kernel does not > know that trick yet). > > The other thing which I have noticed recently is that a lot of > subsystems use a mix of "if (rc) " and "if (rc < 0) ". It's annoying > for Smatch because say a function only returns zero but the some of the > callers check for < 0 and some check for != 0. We can't know for sure > that they are equivalent. > > regards, > dan carpenter Hey Dan, if (rc < 0) and if (rc) pretty much translates to the same thing. It'll only return a negative error value if there are problems and 0 if it succeeds. I feel like the first way is more explicit, since negative numbers are usually used for errors. I've sent a 3rd version of the patch with (rc < 0). And I'm not sure about the way other subsystems use return values. Here it should only either be less than or equal to 0 so it makes sense to me in this circumstance. I ran smatch on my patched file `../smatch/smatch_scripts/kchecker drivers/staging/lustre/lustre/ptlrpc/sec_plain.c` and it didn't find any issues with it. Lidza ^ permalink raw reply [flat|nested] 10+ messages in thread
* [lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference 2016-05-17 14:22 ` Lidza Louina @ 2016-05-17 16:39 ` Dan Carpenter 0 siblings, 0 replies; 10+ messages in thread From: Dan Carpenter @ 2016-05-17 16:39 UTC (permalink / raw) To: lustre-devel On Tue, May 17, 2016 at 10:22:20AM -0400, Lidza Louina wrote: > if (rc < 0) and if (rc) pretty much translates to the same thing. I wasn't talking about this patch in particular; it's just something I have thinking about recently. For example, there are a lot of functions that don't initialize parameters if they return a non-zero value, but the caller checks for negatives. It's often tricky or impossible to determine that these mean the same thing just from analysing the code without making assumptions. I think "if (rc) " is more common than "if (rc < 0)". I also think it's prettier code. But mostly I wish people would do it all consistently. regards, dan carpenter ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-05-17 16:39 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-05-16 14:17 [lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference Lidza Louina 2016-05-16 17:57 ` Patrick Farrell 2016-05-16 18:16 ` James Simmons 2016-05-16 18:22 ` Patrick Farrell 2016-05-16 18:25 ` James Simmons 2016-05-16 18:35 ` Dilger, Andreas 2016-05-16 18:47 ` Lidza Louina 2016-05-17 6:53 ` Dan Carpenter 2016-05-17 14:22 ` Lidza Louina 2016-05-17 16:39 ` Dan Carpenter
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.