[lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference

[lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference

[Lidza Louina]

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

[lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference

[Patrick Farrell]

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;
>   }

[lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference

[James Simmons]

> 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
> 

[lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference

[Patrick Farrell]

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
>>

[lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference

[James Simmons]

> 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
> > >
> 
> 
> 

[lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference

[Dilger, Andreas]

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

[lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference

[Lidza Louina]

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

[lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference

[Dan Carpenter]

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

[lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference

[Lidza Louina]

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

[lustre-devel] [PATCH v2] staging/lustre/ptlrpc: Removes potential null dereference

[Dan Carpenter]

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