| Message ID | 1471910507-83104-1-git-send-email-shiraz.saleem@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, Aug 22, 2016 at 07:01:47PM -0500, Shiraz Saleem wrote: > From: Mustafa Ismail <mustafa.ismail@intel.com> > > 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@redhat.com> > Signed-off-by: Mustafa Ismail <mustafa.ismail@intel.com> > Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com> > --- > > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
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@intel.com> > > > > 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@redhat.com> > > Signed-off-by: Mustafa Ismail <mustafa.ismail@intel.com> > > Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com> > > --- > > > > 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@vger.kernel.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@intel.com> >>> >>> 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@redhat.com> >>> Signed-off-by: Mustafa Ismail <mustafa.ismail@intel.com> >>> Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com> >>> --- >>> >>> 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.
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@intel.com> > >>> > >>> 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@redhat.com> > >>> Signed-off-by: Mustafa Ismail <mustafa.ismail@intel.com> > >>> Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com> > >>> --- > >>> > >>> 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@redhat.com> > GPG Key ID: 0E572FDD >
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@intel.com> > > >>> > > >>> 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@redhat.com> > > >>> Signed-off-by: Mustafa Ismail <mustafa.ismail@intel.com> > > >>> Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com> > > >>> --- > > >>> > > >>> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@intel.com> >>>>>> >>>>>> 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@redhat.com> >>>>>> Signed-off-by: Mustafa Ismail <mustafa.ismail@intel.com> >>>>>> Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com> >>>>>> --- >>>>>> >>>>>> 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.
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@intel.com> > >>>>>> > >>>>>> 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@redhat.com> > >>>>>> Signed-off-by: Mustafa Ismail <mustafa.ismail@intel.com> > >>>>>> Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com> > >>>>>> --- > >>>>>> > >>>>>> 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@redhat.com> > GPG Key ID: 0E572FDD >
On 8/22/2016 8:01 PM, Shiraz Saleem wrote: > From: Mustafa Ismail <mustafa.ismail@intel.com> > > 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@redhat.com> > Signed-off-by: Mustafa Ismail <mustafa.ismail@intel.com> > Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com> > --- > > V2: Fix typo in subject line. Applied with a minor comment added to the file.
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@intel.com> > > >>>>>> > > >>>>>> 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@redhat.com> > > >>>>>> Signed-off-by: Mustafa Ismail <mustafa.ismail@intel.com> > > >>>>>> Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com> > > >>>>>> --- > > >>>>>> > > >>>>>> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }