[PATCH V2] i40iw: Do not set self-referencing pointer to NULL after kfree

[PATCH V2] i40iw: Do not set self-referencing pointer to NULL after kfree

[Shiraz Saleem]

From: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

In i40iw_free_virt_mem(), do not set mem->va to NULL
after freeing it as mem->va is a self-referencing pointer
to mem.

Fixes: 4e9042e647ff ("i40iw: add hw and utils files")

Reported-by: Stefan Assmann <sassmann-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Shiraz Saleem <shiraz.saleem-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---

V2: Fix typo in subject line.

 drivers/infiniband/hw/i40iw/i40iw_utils.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/infiniband/hw/i40iw/i40iw_utils.c b/drivers/infiniband/hw/i40iw/i40iw_utils.c
index 0e8db0a..d5f5de2 100644
--- a/drivers/infiniband/hw/i40iw/i40iw_utils.c
+++ b/drivers/infiniband/hw/i40iw/i40iw_utils.c
@@ -674,7 +674,6 @@ enum i40iw_status_code i40iw_free_virt_mem(struct i40iw_hw *hw,
 	if (!mem)
 		return I40IW_ERR_PARAM;
 	kfree(mem->va);
-	mem->va = NULL;
 	return 0;
 }
 
-- 
2.8.0

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2] i40iw: Do not set self-referencing pointer to NULL after kfree

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1647 bytes --]

On Mon, Aug 22, 2016 at 07:01:47PM -0500, Shiraz Saleem wrote:
> From: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> In i40iw_free_virt_mem(), do not set mem->va to NULL
> after freeing it as mem->va is a self-referencing pointer
> to mem.

Sorry, I failed to understand your commit message and your change.
What did you mean? How do you suppose to use mem->va pointer
after kfree() call on that pointer? Won't you have use-after-free bug in
such case?

> 
> Fixes: 4e9042e647ff ("i40iw: add hw and utils files")
> 
> Reported-by: Stefan Assmann <sassmann-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Shiraz Saleem <shiraz.saleem-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
> 
> V2: Fix typo in subject line.
> 
>  drivers/infiniband/hw/i40iw/i40iw_utils.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/i40iw/i40iw_utils.c b/drivers/infiniband/hw/i40iw/i40iw_utils.c
> index 0e8db0a..d5f5de2 100644
> --- a/drivers/infiniband/hw/i40iw/i40iw_utils.c
> +++ b/drivers/infiniband/hw/i40iw/i40iw_utils.c
> @@ -674,7 +674,6 @@ enum i40iw_status_code i40iw_free_virt_mem(struct i40iw_hw *hw,
>  	if (!mem)
>  		return I40IW_ERR_PARAM;
>  	kfree(mem->va);
> -	mem->va = NULL;
>  	return 0;
>  }
>  
> -- 
> 2.8.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V2] i40iw: Do not set self-referencing pointer to NULL after kfree

[Shiraz Saleem]

On Tue, Aug 23, 2016 at 04:41:35AM +0300, Leon Romanovsky wrote:
> On Mon, Aug 22, 2016 at 07:01:47PM -0500, Shiraz Saleem wrote:
> > From: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > 
> > In i40iw_free_virt_mem(), do not set mem->va to NULL
> > after freeing it as mem->va is a self-referencing pointer
> > to mem.
> 
> Sorry, I failed to understand your commit message and your change.
> What did you mean? How do you suppose to use mem->va pointer
> after kfree() call on that pointer? Won't you have use-after-free bug in
> such case?

The pointer mem->va cannot be used after kfree. But setting it to 
NULL would be writing to freed memory. In i40iw_puda_alloc_buf(), 
when a buffer is allocated of type struct i40iw_puda_buf, the address 
of the buffer itself is stored within the structure (in member buf_mem). 
When this pointer is freed, the structure containing the pointer is freed,
so writing to the structure would be writing to freed memory, which is 
what this fix is for.
 
> > 
> > Fixes: 4e9042e647ff ("i40iw: add hw and utils files")
> > 
> > Reported-by: Stefan Assmann <sassmann-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Shiraz Saleem <shiraz.saleem-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > ---
> > 
> > V2: Fix typo in subject line.
> > 
> >  drivers/infiniband/hw/i40iw/i40iw_utils.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/infiniband/hw/i40iw/i40iw_utils.c b/drivers/infiniband/hw/i40iw/i40iw_utils.c
> > index 0e8db0a..d5f5de2 100644
> > --- a/drivers/infiniband/hw/i40iw/i40iw_utils.c
> > +++ b/drivers/infiniband/hw/i40iw/i40iw_utils.c
> > @@ -674,7 +674,6 @@ enum i40iw_status_code i40iw_free_virt_mem(struct i40iw_hw *hw,
> >  	if (!mem)
> >  		return I40IW_ERR_PARAM;
> >  	kfree(mem->va);
> > -	mem->va = NULL;
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.8.0
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2] i40iw: Do not set self-referencing pointer to NULL after kfree

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 2748 bytes --]

On 8/23/2016 12:00 PM, Shiraz Saleem wrote:
> On Tue, Aug 23, 2016 at 04:41:35AM +0300, Leon Romanovsky wrote:
>> On Mon, Aug 22, 2016 at 07:01:47PM -0500, Shiraz Saleem wrote:
>>> From: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>>
>>> In i40iw_free_virt_mem(), do not set mem->va to NULL
>>> after freeing it as mem->va is a self-referencing pointer
>>> to mem.
>>
>> Sorry, I failed to understand your commit message and your change.
>> What did you mean? How do you suppose to use mem->va pointer
>> after kfree() call on that pointer? Won't you have use-after-free bug in
>> such case?
> 
> The pointer mem->va cannot be used after kfree. But setting it to 
> NULL would be writing to freed memory. In i40iw_puda_alloc_buf(), 
> when a buffer is allocated of type struct i40iw_puda_buf, the address 
> of the buffer itself is stored within the structure (in member buf_mem). 
> When this pointer is freed, the structure containing the pointer is freed,
> so writing to the structure would be writing to freed memory, which is 
> what this fix is for.
>  
>>>
>>> Fixes: 4e9042e647ff ("i40iw: add hw and utils files")
>>>
>>> Reported-by: Stefan Assmann <sassmann-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> Signed-off-by: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>> Signed-off-by: Shiraz Saleem <shiraz.saleem-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>> ---
>>>
>>> V2: Fix typo in subject line.
>>>
>>>  drivers/infiniband/hw/i40iw/i40iw_utils.c | 1 -
>>>  1 file changed, 1 deletion(-)
>>>
>>> diff --git a/drivers/infiniband/hw/i40iw/i40iw_utils.c b/drivers/infiniband/hw/i40iw/i40iw_utils.c
>>> index 0e8db0a..d5f5de2 100644
>>> --- a/drivers/infiniband/hw/i40iw/i40iw_utils.c
>>> +++ b/drivers/infiniband/hw/i40iw/i40iw_utils.c
>>> @@ -674,7 +674,6 @@ enum i40iw_status_code i40iw_free_virt_mem(struct i40iw_hw *hw,
>>>  	if (!mem)
>>>  		return I40IW_ERR_PARAM;
>>>  	kfree(mem->va);
>>> -	mem->va = NULL;
>>>  	return 0;

Your commit message is a bit hard to follow, but if I follow the
conversation, kfree(mem->va) is the same as kfree(mem), is it not?  If
it is, couldn't you just kfree(mem)?  That would avoid the confusion
here.  Also, if it matters, you can use a temporary pointer, aka:

p = mem->va;
mem->va = NULL;
kfree(p);

but, again, if mem->va is just a self-referencing pointer back to mem,
then why not just kfree(mem)?  I'm concerned that this convoluted way of
doing things will come back to haunt us later when people think they are
submitting a fix and simply reintroduce the same bug again.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: 0E572FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH V2] i40iw: Do not set self-referencing pointer to NULL after kfree

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2984 bytes --]

On Tue, Aug 23, 2016 at 12:51:10PM -0400, Doug Ledford wrote:
> On 8/23/2016 12:00 PM, Shiraz Saleem wrote:
> > On Tue, Aug 23, 2016 at 04:41:35AM +0300, Leon Romanovsky wrote:
> >> On Mon, Aug 22, 2016 at 07:01:47PM -0500, Shiraz Saleem wrote:
> >>> From: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >>>
> >>> In i40iw_free_virt_mem(), do not set mem->va to NULL
> >>> after freeing it as mem->va is a self-referencing pointer
> >>> to mem.
> >>
> >> Sorry, I failed to understand your commit message and your change.
> >> What did you mean? How do you suppose to use mem->va pointer
> >> after kfree() call on that pointer? Won't you have use-after-free bug in
> >> such case?
> > 
> > The pointer mem->va cannot be used after kfree. But setting it to 
> > NULL would be writing to freed memory. In i40iw_puda_alloc_buf(), 
> > when a buffer is allocated of type struct i40iw_puda_buf, the address 
> > of the buffer itself is stored within the structure (in member buf_mem). 
> > When this pointer is freed, the structure containing the pointer is freed,
> > so writing to the structure would be writing to freed memory, which is 
> > what this fix is for.
> >  
> >>>
> >>> Fixes: 4e9042e647ff ("i40iw: add hw and utils files")
> >>>
> >>> Reported-by: Stefan Assmann <sassmann-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>> Signed-off-by: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >>> Signed-off-by: Shiraz Saleem <shiraz.saleem-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >>> ---
> >>>
> >>> V2: Fix typo in subject line.
> >>>
> >>>  drivers/infiniband/hw/i40iw/i40iw_utils.c | 1 -
> >>>  1 file changed, 1 deletion(-)
> >>>
> >>> diff --git a/drivers/infiniband/hw/i40iw/i40iw_utils.c b/drivers/infiniband/hw/i40iw/i40iw_utils.c
> >>> index 0e8db0a..d5f5de2 100644
> >>> --- a/drivers/infiniband/hw/i40iw/i40iw_utils.c
> >>> +++ b/drivers/infiniband/hw/i40iw/i40iw_utils.c
> >>> @@ -674,7 +674,6 @@ enum i40iw_status_code i40iw_free_virt_mem(struct i40iw_hw *hw,
> >>>  	if (!mem)
> >>>  		return I40IW_ERR_PARAM;
> >>>  	kfree(mem->va);
> >>> -	mem->va = NULL;
> >>>  	return 0;
> 
> Your commit message is a bit hard to follow, but if I follow the
> conversation, kfree(mem->va) is the same as kfree(mem), is it not?  If
> it is, couldn't you just kfree(mem)?  That would avoid the confusion
> here.  Also, if it matters, you can use a temporary pointer, aka:
> 
> p = mem->va;
> mem->va = NULL;
> kfree(p);
> 
> but, again, if mem->va is just a self-referencing pointer back to mem,
> then why not just kfree(mem)?  I'm concerned that this convoluted way of
> doing things will come back to haunt us later when people think they are
> submitting a fix and simply reintroduce the same bug again.

Yeah, I totally agree with you.

> 
> 
> -- 
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG Key ID: 0E572FDD
> 




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V2] i40iw: Do not set self-referencing pointer to NULL after kfree

[Shiraz Saleem]

On Tue, Aug 23, 2016 at 09:46:23PM +0300, Leon Romanovsky wrote:
> On Tue, Aug 23, 2016 at 12:51:10PM -0400, Doug Ledford wrote:
> > On 8/23/2016 12:00 PM, Shiraz Saleem wrote:
> > > On Tue, Aug 23, 2016 at 04:41:35AM +0300, Leon Romanovsky wrote:
> > >> On Mon, Aug 22, 2016 at 07:01:47PM -0500, Shiraz Saleem wrote:
> > >>> From: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > >>>
> > >>> In i40iw_free_virt_mem(), do not set mem->va to NULL
> > >>> after freeing it as mem->va is a self-referencing pointer
> > >>> to mem.
> > >>
> > >> Sorry, I failed to understand your commit message and your change.
> > >> What did you mean? How do you suppose to use mem->va pointer
> > >> after kfree() call on that pointer? Won't you have use-after-free bug in
> > >> such case?
> > > 
> > > The pointer mem->va cannot be used after kfree. But setting it to 
> > > NULL would be writing to freed memory. In i40iw_puda_alloc_buf(), 
> > > when a buffer is allocated of type struct i40iw_puda_buf, the address 
> > > of the buffer itself is stored within the structure (in member buf_mem). 
> > > When this pointer is freed, the structure containing the pointer is freed,
> > > so writing to the structure would be writing to freed memory, which is 
> > > what this fix is for.
> > >  
> > >>>
> > >>> Fixes: 4e9042e647ff ("i40iw: add hw and utils files")
> > >>>
> > >>> Reported-by: Stefan Assmann <sassmann-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > >>> Signed-off-by: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > >>> Signed-off-by: Shiraz Saleem <shiraz.saleem-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > >>> ---
> > >>>
> > >>> V2: Fix typo in subject line.
> > >>>
> > >>>  drivers/infiniband/hw/i40iw/i40iw_utils.c | 1 -
> > >>>  1 file changed, 1 deletion(-)
> > >>>
> > >>> diff --git a/drivers/infiniband/hw/i40iw/i40iw_utils.c b/drivers/infiniband/hw/i40iw/i40iw_utils.c
> > >>> index 0e8db0a..d5f5de2 100644
> > >>> --- a/drivers/infiniband/hw/i40iw/i40iw_utils.c
> > >>> +++ b/drivers/infiniband/hw/i40iw/i40iw_utils.c
> > >>> @@ -674,7 +674,6 @@ enum i40iw_status_code i40iw_free_virt_mem(struct i40iw_hw *hw,
> > >>>  	if (!mem)
> > >>>  		return I40IW_ERR_PARAM;
> > >>>  	kfree(mem->va);
> > >>> -	mem->va = NULL;
> > >>>  	return 0;
> > 
> > Your commit message is a bit hard to follow, but if I follow the
> > conversation, kfree(mem->va) is the same as kfree(mem), is it not?  If
> > it is, couldn't you just kfree(mem)?  That would avoid the confusion
> > here.  Also, if it matters, you can use a temporary pointer, aka:
> > 
> > p = mem->va;
> > mem->va = NULL;
> > kfree(p);
> > 
> > but, again, if mem->va is just a self-referencing pointer back to mem,
> > then why not just kfree(mem)?  I'm concerned that this convoluted way of
> > doing things will come back to haunt us later when people think they are
> > submitting a fix and simply reintroduce the same bug again.
> 
> Yeah, I totally agree with you.
> 

OK, I see the the possible confusion of the commit message. 

Maybe the following is clearer:
"In i40iw_free_virt_mem(), do not set mem->va to NULL after freeing it, as mem->va is a pointer 
to the structure containing mem"

So kfree(mem->va) is not the same as kfree(mem). 


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2] i40iw: Do not set self-referencing pointer to NULL after kfree

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 3636 bytes --]

On 8/23/2016 3:00 PM, Shiraz Saleem wrote:
> On Tue, Aug 23, 2016 at 09:46:23PM +0300, Leon Romanovsky wrote:
>> On Tue, Aug 23, 2016 at 12:51:10PM -0400, Doug Ledford wrote:
>>> On 8/23/2016 12:00 PM, Shiraz Saleem wrote:
>>>> On Tue, Aug 23, 2016 at 04:41:35AM +0300, Leon Romanovsky wrote:
>>>>> On Mon, Aug 22, 2016 at 07:01:47PM -0500, Shiraz Saleem wrote:
>>>>>> From: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>>>>>
>>>>>> In i40iw_free_virt_mem(), do not set mem->va to NULL
>>>>>> after freeing it as mem->va is a self-referencing pointer
>>>>>> to mem.
>>>>>
>>>>> Sorry, I failed to understand your commit message and your change.
>>>>> What did you mean? How do you suppose to use mem->va pointer
>>>>> after kfree() call on that pointer? Won't you have use-after-free bug in
>>>>> such case?
>>>>
>>>> The pointer mem->va cannot be used after kfree. But setting it to 
>>>> NULL would be writing to freed memory. In i40iw_puda_alloc_buf(), 
>>>> when a buffer is allocated of type struct i40iw_puda_buf, the address 
>>>> of the buffer itself is stored within the structure (in member buf_mem). 
>>>> When this pointer is freed, the structure containing the pointer is freed,
>>>> so writing to the structure would be writing to freed memory, which is 
>>>> what this fix is for.
>>>>  
>>>>>>
>>>>>> Fixes: 4e9042e647ff ("i40iw: add hw and utils files")
>>>>>>
>>>>>> Reported-by: Stefan Assmann <sassmann-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>>>> Signed-off-by: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>>>>> Signed-off-by: Shiraz Saleem <shiraz.saleem-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>>>>> ---
>>>>>>
>>>>>> V2: Fix typo in subject line.
>>>>>>
>>>>>>  drivers/infiniband/hw/i40iw/i40iw_utils.c | 1 -
>>>>>>  1 file changed, 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/infiniband/hw/i40iw/i40iw_utils.c b/drivers/infiniband/hw/i40iw/i40iw_utils.c
>>>>>> index 0e8db0a..d5f5de2 100644
>>>>>> --- a/drivers/infiniband/hw/i40iw/i40iw_utils.c
>>>>>> +++ b/drivers/infiniband/hw/i40iw/i40iw_utils.c
>>>>>> @@ -674,7 +674,6 @@ enum i40iw_status_code i40iw_free_virt_mem(struct i40iw_hw *hw,
>>>>>>  	if (!mem)
>>>>>>  		return I40IW_ERR_PARAM;
>>>>>>  	kfree(mem->va);
>>>>>> -	mem->va = NULL;
>>>>>>  	return 0;
>>>
>>> Your commit message is a bit hard to follow, but if I follow the
>>> conversation, kfree(mem->va) is the same as kfree(mem), is it not?  If
>>> it is, couldn't you just kfree(mem)?  That would avoid the confusion
>>> here.  Also, if it matters, you can use a temporary pointer, aka:
>>>
>>> p = mem->va;
>>> mem->va = NULL;
>>> kfree(p);
>>>
>>> but, again, if mem->va is just a self-referencing pointer back to mem,
>>> then why not just kfree(mem)?  I'm concerned that this convoluted way of
>>> doing things will come back to haunt us later when people think they are
>>> submitting a fix and simply reintroduce the same bug again.
>>
>> Yeah, I totally agree with you.
>>
> 
> OK, I see the the possible confusion of the commit message. 
> 
> Maybe the following is clearer:
> "In i40iw_free_virt_mem(), do not set mem->va to NULL after freeing it, as mem->va is a pointer 
> to the structure containing mem"
> 
> So kfree(mem->va) is not the same as kfree(mem). 

That makes more sense.  I still think things are a bit convoluted here,
and maybe even a code comment is in order.  I'll probably change it up a
little bit as I pull it in.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: 0E572FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH V2] i40iw: Do not set self-referencing pointer to NULL after kfree

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 3970 bytes --]

On Tue, Aug 23, 2016 at 03:05:42PM -0400, Doug Ledford wrote:
> On 8/23/2016 3:00 PM, Shiraz Saleem wrote:
> > On Tue, Aug 23, 2016 at 09:46:23PM +0300, Leon Romanovsky wrote:
> >> On Tue, Aug 23, 2016 at 12:51:10PM -0400, Doug Ledford wrote:
> >>> On 8/23/2016 12:00 PM, Shiraz Saleem wrote:
> >>>> On Tue, Aug 23, 2016 at 04:41:35AM +0300, Leon Romanovsky wrote:
> >>>>> On Mon, Aug 22, 2016 at 07:01:47PM -0500, Shiraz Saleem wrote:
> >>>>>> From: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >>>>>>
> >>>>>> In i40iw_free_virt_mem(), do not set mem->va to NULL
> >>>>>> after freeing it as mem->va is a self-referencing pointer
> >>>>>> to mem.
> >>>>>
> >>>>> Sorry, I failed to understand your commit message and your change.
> >>>>> What did you mean? How do you suppose to use mem->va pointer
> >>>>> after kfree() call on that pointer? Won't you have use-after-free bug in
> >>>>> such case?
> >>>>
> >>>> The pointer mem->va cannot be used after kfree. But setting it to 
> >>>> NULL would be writing to freed memory. In i40iw_puda_alloc_buf(), 
> >>>> when a buffer is allocated of type struct i40iw_puda_buf, the address 
> >>>> of the buffer itself is stored within the structure (in member buf_mem). 
> >>>> When this pointer is freed, the structure containing the pointer is freed,
> >>>> so writing to the structure would be writing to freed memory, which is 
> >>>> what this fix is for.
> >>>>  
> >>>>>>
> >>>>>> Fixes: 4e9042e647ff ("i40iw: add hw and utils files")
> >>>>>>
> >>>>>> Reported-by: Stefan Assmann <sassmann-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>>>>> Signed-off-by: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >>>>>> Signed-off-by: Shiraz Saleem <shiraz.saleem-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >>>>>> ---
> >>>>>>
> >>>>>> V2: Fix typo in subject line.
> >>>>>>
> >>>>>>  drivers/infiniband/hw/i40iw/i40iw_utils.c | 1 -
> >>>>>>  1 file changed, 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/drivers/infiniband/hw/i40iw/i40iw_utils.c b/drivers/infiniband/hw/i40iw/i40iw_utils.c
> >>>>>> index 0e8db0a..d5f5de2 100644
> >>>>>> --- a/drivers/infiniband/hw/i40iw/i40iw_utils.c
> >>>>>> +++ b/drivers/infiniband/hw/i40iw/i40iw_utils.c
> >>>>>> @@ -674,7 +674,6 @@ enum i40iw_status_code i40iw_free_virt_mem(struct i40iw_hw *hw,
> >>>>>>  	if (!mem)
> >>>>>>  		return I40IW_ERR_PARAM;
> >>>>>>  	kfree(mem->va);
> >>>>>> -	mem->va = NULL;
> >>>>>>  	return 0;
> >>>
> >>> Your commit message is a bit hard to follow, but if I follow the
> >>> conversation, kfree(mem->va) is the same as kfree(mem), is it not?  If
> >>> it is, couldn't you just kfree(mem)?  That would avoid the confusion
> >>> here.  Also, if it matters, you can use a temporary pointer, aka:
> >>>
> >>> p = mem->va;
> >>> mem->va = NULL;
> >>> kfree(p);
> >>>
> >>> but, again, if mem->va is just a self-referencing pointer back to mem,
> >>> then why not just kfree(mem)?  I'm concerned that this convoluted way of
> >>> doing things will come back to haunt us later when people think they are
> >>> submitting a fix and simply reintroduce the same bug again.
> >>
> >> Yeah, I totally agree with you.
> >>
> > 
> > OK, I see the the possible confusion of the commit message. 
> > 
> > Maybe the following is clearer:
> > "In i40iw_free_virt_mem(), do not set mem->va to NULL after freeing it, as mem->va is a pointer 
> > to the structure containing mem"
> > 
> > So kfree(mem->va) is not the same as kfree(mem). 
> 
> That makes more sense.  I still think things are a bit convoluted here,
> and maybe even a code comment is in order.  I'll probably change it up a
> little bit as I pull it in.

I would like to see code snippet of such struct and/or code which is
depend on it.

Thanks

> 
> 
> -- 
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG Key ID: 0E572FDD
> 




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V2] i40iw: Do not set self-referencing pointer to NULL after kfree

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 779 bytes --]

On 8/22/2016 8:01 PM, Shiraz Saleem wrote:
> From: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> In i40iw_free_virt_mem(), do not set mem->va to NULL
> after freeing it as mem->va is a self-referencing pointer
> to mem.
> 
> Fixes: 4e9042e647ff ("i40iw: add hw and utils files")
> 
> Reported-by: Stefan Assmann <sassmann-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Shiraz Saleem <shiraz.saleem-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
> 
> V2: Fix typo in subject line.

Applied with a minor comment added to the file.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: 0E572FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH V2] i40iw: Do not set self-referencing pointer to NULL after kfree

[Shiraz Saleem]

On Wed, Aug 24, 2016 at 12:03:47AM +0300, Leon Romanovsky wrote:
> On Tue, Aug 23, 2016 at 03:05:42PM -0400, Doug Ledford wrote:
> > On 8/23/2016 3:00 PM, Shiraz Saleem wrote:
> > > On Tue, Aug 23, 2016 at 09:46:23PM +0300, Leon Romanovsky wrote:
> > >> On Tue, Aug 23, 2016 at 12:51:10PM -0400, Doug Ledford wrote:
> > >>> On 8/23/2016 12:00 PM, Shiraz Saleem wrote:
> > >>>> On Tue, Aug 23, 2016 at 04:41:35AM +0300, Leon Romanovsky wrote:
> > >>>>> On Mon, Aug 22, 2016 at 07:01:47PM -0500, Shiraz Saleem wrote:
> > >>>>>> From: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > >>>>>>
> > >>>>>> In i40iw_free_virt_mem(), do not set mem->va to NULL
> > >>>>>> after freeing it as mem->va is a self-referencing pointer
> > >>>>>> to mem.
> > >>>>>
> > >>>>> Sorry, I failed to understand your commit message and your change.
> > >>>>> What did you mean? How do you suppose to use mem->va pointer
> > >>>>> after kfree() call on that pointer? Won't you have use-after-free bug in
> > >>>>> such case?
> > >>>>
> > >>>> The pointer mem->va cannot be used after kfree. But setting it to 
> > >>>> NULL would be writing to freed memory. In i40iw_puda_alloc_buf(), 
> > >>>> when a buffer is allocated of type struct i40iw_puda_buf, the address 
> > >>>> of the buffer itself is stored within the structure (in member buf_mem). 
> > >>>> When this pointer is freed, the structure containing the pointer is freed,
> > >>>> so writing to the structure would be writing to freed memory, which is 
> > >>>> what this fix is for.
> > >>>>  
> > >>>>>>
> > >>>>>> Fixes: 4e9042e647ff ("i40iw: add hw and utils files")
> > >>>>>>
> > >>>>>> Reported-by: Stefan Assmann <sassmann-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > >>>>>> Signed-off-by: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > >>>>>> Signed-off-by: Shiraz Saleem <shiraz.saleem-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > >>>>>> ---
> > >>>>>>
> > >>>>>> V2: Fix typo in subject line.
> > >>>>>>
> > >>>>>>  drivers/infiniband/hw/i40iw/i40iw_utils.c | 1 -
> > >>>>>>  1 file changed, 1 deletion(-)
> > >>>>>>
> > >>>>>> diff --git a/drivers/infiniband/hw/i40iw/i40iw_utils.c b/drivers/infiniband/hw/i40iw/i40iw_utils.c
> > >>>>>> index 0e8db0a..d5f5de2 100644
> > >>>>>> --- a/drivers/infiniband/hw/i40iw/i40iw_utils.c
> > >>>>>> +++ b/drivers/infiniband/hw/i40iw/i40iw_utils.c
> > >>>>>> @@ -674,7 +674,6 @@ enum i40iw_status_code i40iw_free_virt_mem(struct i40iw_hw *hw,
> > >>>>>>  	if (!mem)
> > >>>>>>  		return I40IW_ERR_PARAM;
> > >>>>>>  	kfree(mem->va);
> > >>>>>> -	mem->va = NULL;
> > >>>>>>  	return 0;
> > >>>
> > >>> Your commit message is a bit hard to follow, but if I follow the
> > >>> conversation, kfree(mem->va) is the same as kfree(mem), is it not?  If
> > >>> it is, couldn't you just kfree(mem)?  That would avoid the confusion
> > >>> here.  Also, if it matters, you can use a temporary pointer, aka:
> > >>>
> > >>> p = mem->va;
> > >>> mem->va = NULL;
> > >>> kfree(p);
> > >>>
> > >>> but, again, if mem->va is just a self-referencing pointer back to mem,
> > >>> then why not just kfree(mem)?  I'm concerned that this convoluted way of
> > >>> doing things will come back to haunt us later when people think they are
> > >>> submitting a fix and simply reintroduce the same bug again.
> > >>
> > >> Yeah, I totally agree with you.
> > >>
> > > 
> > > OK, I see the the possible confusion of the commit message. 
> > > 
> > > Maybe the following is clearer:
> > > "In i40iw_free_virt_mem(), do not set mem->va to NULL after freeing it, as mem->va is a pointer 
> > > to the structure containing mem"
> > > 
> > > So kfree(mem->va) is not the same as kfree(mem). 
> > 
> > That makes more sense.  I still think things are a bit convoluted here,
> > and maybe even a code comment is in order.  I'll probably change it up a
> > little bit as I pull it in.
> 
> I would like to see code snippet of such struct and/or code which is
> depend on it.
> 

static struct i40iw_puda_buf *i40iw_puda_alloc_buf(struct i40iw_sc_dev *dev,
                                                   u32 length)
{
 ------>struct i40iw_puda_buf *buf = NULL;
 ------>struct i40iw_virt_mem buf_mem;
        enum i40iw_status_code ret;

 ------->ret = i40iw_allocate_virt_mem(dev->hw, &buf_mem,
                                      sizeof(struct i40iw_puda_buf));
        if (ret) {
                i40iw_debug(dev, I40IW_DEBUG_PUDA,
                            "%s: error mem for buf\n", __func__);
                return NULL;
        }
-------->buf = (struct i40iw_puda_buf *)buf_mem.va;
        ret = i40iw_allocate_dma_mem(dev->hw, &buf->mem, length, 1);
        if (ret) {
                i40iw_debug(dev, I40IW_DEBUG_PUDA,
                            "%s: error dma mem for buf\n", __func__);
                i40iw_free_virt_mem(dev->hw, &buf_mem);
                return NULL;
        }
-------->buf->buf_mem.va = buf_mem.va;
        buf->buf_mem.size = buf_mem.size;
        return buf;
}
}

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html