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@xxxxxxxxx>
> >>>>>>
> >>>>>> 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@xxxxxxxxxx>
> >>>>>> Signed-off-by: Mustafa Ismail <mustafa.ismail@xxxxxxxxx>
> >>>>>> Signed-off-by: Shiraz Saleem <shiraz.saleem@xxxxxxxxx>
> >>>>>> ---
> >>>>>>
> >>>>>> 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@xxxxxxxxxx>
> GPG Key ID: 0E572FDD
>
Attachment:
signature.asc
Description: PGP signature
