* [PATCH linux-next v2 1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device() [not found] <20230309040107.534716-1-dzm91@hust.edu.cn> @ 2023-03-09 4:01 ` Dongliang Mu 2023-03-14 14:22 ` Dan Carpenter ` (2 more replies) 2023-03-09 4:01 ` [PATCH linux-next v2 2/3] drivers/platform/x86/intel/tpmi: revise the comment of intel_vsec_add_aux Dongliang Mu 2023-03-09 4:01 ` [PATCH linux-next v2 3/3] drivers/platform/x86/intel: fix a memory leak in intel_vsec_add_aux Dongliang Mu 2 siblings, 3 replies; 18+ messages in thread From: Dongliang Mu @ 2023-03-09 4:01 UTC (permalink / raw) To: Srinivas Pandruvada, Hans de Goede, Mark Gross Cc: Dongliang Mu, platform-driver-x86, linux-kernel The previous commit 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free reported by Smatch") incorrectly handle the deallocation of res variable. As shown in the comment, intel_vsec_add_aux handles all the deallocation of res and feature_vsec_dev. Therefore, kfree(res) can still cause double free if intel_vsec_add_aux returns error. Fix this by adjusting the error handling part in tpmi_create_device, following the function intel_vsec_add_dev. Fixes: 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free reported by Smatch") Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn> --- drivers/platform/x86/intel/tpmi.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c index c999732b0f1e..882fe5e4763f 100644 --- a/drivers/platform/x86/intel/tpmi.c +++ b/drivers/platform/x86/intel/tpmi.c @@ -215,8 +215,8 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info, feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev), GFP_KERNEL); if (!feature_vsec_dev) { - ret = -ENOMEM; - goto free_res; + kfree(res); + return -ENOMEM; } snprintf(feature_id_name, sizeof(feature_id_name), "tpmi-%s", name); @@ -242,17 +242,8 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info, * feature_vsec_dev memory is also freed as part of device * delete. */ - ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev, - feature_vsec_dev, feature_id_name); - if (ret) - goto free_res; - - return 0; - -free_res: - kfree(res); - - return ret; + return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev, + feature_vsec_dev, feature_id_name); } static int tpmi_create_devices(struct intel_tpmi_info *tpmi_info) -- 2.39.2 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH linux-next v2 1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device() 2023-03-09 4:01 ` [PATCH linux-next v2 1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device() Dongliang Mu @ 2023-03-14 14:22 ` Dan Carpenter 2023-03-16 14:25 ` Hans de Goede 2023-03-20 10:32 ` Hans de Goede 2 siblings, 0 replies; 18+ messages in thread From: Dan Carpenter @ 2023-03-14 14:22 UTC (permalink / raw) To: Dongliang Mu Cc: Srinivas Pandruvada, Hans de Goede, Mark Gross, platform-driver-x86, linux-kernel On Thu, Mar 09, 2023 at 12:01:05PM +0800, Dongliang Mu wrote: > The previous commit 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix > double free reported by Smatch") incorrectly handle the deallocation of > res variable. As shown in the comment, intel_vsec_add_aux handles all > the deallocation of res and feature_vsec_dev. Therefore, kfree(res) can > still cause double free if intel_vsec_add_aux returns error. > > Fix this by adjusting the error handling part in tpmi_create_device, > following the function intel_vsec_add_dev. > > Fixes: 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free reported by Smatch") > Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn> > --- Yeah. These patches are right. The earlier fix still has a double free. Devres stuff is confusing... Reviewed-by: Dan Carpenter <error27@gmail.com> regards, dan carpenter ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH linux-next v2 1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device() 2023-03-09 4:01 ` [PATCH linux-next v2 1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device() Dongliang Mu 2023-03-14 14:22 ` Dan Carpenter @ 2023-03-16 14:25 ` Hans de Goede 2023-03-16 18:18 ` srinivas pandruvada 2023-03-20 10:32 ` Hans de Goede 2 siblings, 1 reply; 18+ messages in thread From: Hans de Goede @ 2023-03-16 14:25 UTC (permalink / raw) To: Dongliang Mu, Srinivas Pandruvada, Mark Gross Cc: platform-driver-x86, linux-kernel Hi, On 3/9/23 05:01, Dongliang Mu wrote: > The previous commit 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix > double free reported by Smatch") incorrectly handle the deallocation of > res variable. As shown in the comment, intel_vsec_add_aux handles all > the deallocation of res and feature_vsec_dev. Therefore, kfree(res) can > still cause double free if intel_vsec_add_aux returns error. > > Fix this by adjusting the error handling part in tpmi_create_device, > following the function intel_vsec_add_dev. > > Fixes: 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free reported by Smatch") > Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn> IIRC then after this v2 was posted I still saw some comments on the original v1 which was not posted on the list. Without the v1 comments being on the list and this archived, I have lost track of what the status of these patches is. Srinivas, can you let me know if I should merge these, or if more changes are necessary ? From the off-list discussion of v1 I got the impression more changes are necessary, but I'm not sure. Regards, Hans > --- > drivers/platform/x86/intel/tpmi.c | 17 ++++------------- > 1 file changed, 4 insertions(+), 13 deletions(-) > > diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c > index c999732b0f1e..882fe5e4763f 100644 > --- a/drivers/platform/x86/intel/tpmi.c > +++ b/drivers/platform/x86/intel/tpmi.c > @@ -215,8 +215,8 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info, > > feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev), GFP_KERNEL); > if (!feature_vsec_dev) { > - ret = -ENOMEM; > - goto free_res; > + kfree(res); > + return -ENOMEM; > } > > snprintf(feature_id_name, sizeof(feature_id_name), "tpmi-%s", name); > @@ -242,17 +242,8 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info, > * feature_vsec_dev memory is also freed as part of device > * delete. > */ > - ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev, > - feature_vsec_dev, feature_id_name); > - if (ret) > - goto free_res; > - > - return 0; > - > -free_res: > - kfree(res); > - > - return ret; > + return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev, > + feature_vsec_dev, feature_id_name); > } > > static int tpmi_create_devices(struct intel_tpmi_info *tpmi_info) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH linux-next v2 1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device() 2023-03-16 14:25 ` Hans de Goede @ 2023-03-16 18:18 ` srinivas pandruvada 2023-03-17 1:28 ` Dongliang Mu 0 siblings, 1 reply; 18+ messages in thread From: srinivas pandruvada @ 2023-03-16 18:18 UTC (permalink / raw) To: Hans de Goede, Dongliang Mu, Mark Gross; +Cc: platform-driver-x86, linux-kernel Hi Hans, On Thu, 2023-03-16 at 15:25 +0100, Hans de Goede wrote: > Hi, > > On 3/9/23 05:01, Dongliang Mu wrote: > > The previous commit 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix > > double free reported by Smatch") incorrectly handle the > > deallocation of > > res variable. As shown in the comment, intel_vsec_add_aux handles > > all > > the deallocation of res and feature_vsec_dev. Therefore, kfree(res) > > can > > still cause double free if intel_vsec_add_aux returns error. > > > > Fix this by adjusting the error handling part in > > tpmi_create_device, > > following the function intel_vsec_add_dev. > > > > Fixes: 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free > > reported by Smatch") > > Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > > IIRC then after this v2 was posted I still saw some comments on the > original v1 which was not posted on the list. Without the v1 comments > being on the list and this archived, I have lost track of what the > status of these patches is. > > Srinivas, can you let me know if I should merge these, or if more > changes are necessary ? > > From the off-list discussion of v1 I got the impression more changes > are necessary, but I'm not sure. I was looking for changes submitted by the following patch " [PATCH linux-next v2 3/3] drivers/platform/x86/intel: fix a memory leak in intel_vsec_add_aux " Since I was not copied on this, I was unaware. So I was requesting this change. Thanks, Srinivas > > Regards, > > Hans > > > > > > --- > > drivers/platform/x86/intel/tpmi.c | 17 ++++------------- > > 1 file changed, 4 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/platform/x86/intel/tpmi.c > > b/drivers/platform/x86/intel/tpmi.c > > index c999732b0f1e..882fe5e4763f 100644 > > --- a/drivers/platform/x86/intel/tpmi.c > > +++ b/drivers/platform/x86/intel/tpmi.c > > @@ -215,8 +215,8 @@ static int tpmi_create_device(struct > > intel_tpmi_info *tpmi_info, > > > > feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev), > > GFP_KERNEL); > > if (!feature_vsec_dev) { > > - ret = -ENOMEM; > > - goto free_res; > > + kfree(res); > > + return -ENOMEM; > > } > > > > snprintf(feature_id_name, sizeof(feature_id_name), "tpmi- > > %s", name); > > @@ -242,17 +242,8 @@ static int tpmi_create_device(struct > > intel_tpmi_info *tpmi_info, > > * feature_vsec_dev memory is also freed as part of device > > * delete. > > */ > > - ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- > > >auxdev.dev, > > - feature_vsec_dev, > > feature_id_name); > > - if (ret) > > - goto free_res; > > - > > - return 0; > > - > > -free_res: > > - kfree(res); > > - > > - return ret; > > + return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- > > >auxdev.dev, > > + feature_vsec_dev, > > feature_id_name); > > } > > > > static int tpmi_create_devices(struct intel_tpmi_info *tpmi_info) > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH linux-next v2 1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device() 2023-03-16 18:18 ` srinivas pandruvada @ 2023-03-17 1:28 ` Dongliang Mu 2023-03-17 8:51 ` Hans de Goede 2023-03-17 10:27 ` srinivas pandruvada 0 siblings, 2 replies; 18+ messages in thread From: Dongliang Mu @ 2023-03-17 1:28 UTC (permalink / raw) To: srinivas pandruvada, Hans de Goede, Mark Gross Cc: platform-driver-x86, linux-kernel On 2023/3/17 02:18, srinivas pandruvada wrote: > Hi Hans, > > On Thu, 2023-03-16 at 15:25 +0100, Hans de Goede wrote: >> Hi, >> >> On 3/9/23 05:01, Dongliang Mu wrote: >>> The previous commit 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix >>> double free reported by Smatch") incorrectly handle the >>> deallocation of >>> res variable. As shown in the comment, intel_vsec_add_aux handles >>> all >>> the deallocation of res and feature_vsec_dev. Therefore, kfree(res) >>> can >>> still cause double free if intel_vsec_add_aux returns error. >>> >>> Fix this by adjusting the error handling part in >>> tpmi_create_device, >>> following the function intel_vsec_add_dev. >>> >>> Fixes: 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free >>> reported by Smatch") >>> Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn> > Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > >> IIRC then after this v2 was posted I still saw some comments on the >> original v1 which was not posted on the list. Without the v1 comments >> being on the list and this archived, I have lost track of what the >> status of these patches is. >> >> Srinivas, can you let me know if I should merge these, or if more >> changes are necessary ? >> >> From the off-list discussion of v1 I got the impression more changes >> are necessary, but I'm not sure. > I was looking for changes submitted by the following patch > " > [PATCH linux-next v2 3/3] drivers/platform/x86/intel: fix a memory leak > in intel_vsec_add_aux > " > > Since I was not copied on this, I was unaware. So I was requesting this > change. > > Thanks, > Srinivas Hi Srinivas and Hans, How about folding these three patches into one patch and resend a v3 patch? This will get all people together and avoid the previous embarrassing sitation. Dongliang Mu > >> Regards, >> >> Hans >> >> >> >> >>> --- >>> drivers/platform/x86/intel/tpmi.c | 17 ++++------------- >>> 1 file changed, 4 insertions(+), 13 deletions(-) >>> >>> diff --git a/drivers/platform/x86/intel/tpmi.c >>> b/drivers/platform/x86/intel/tpmi.c >>> index c999732b0f1e..882fe5e4763f 100644 >>> --- a/drivers/platform/x86/intel/tpmi.c >>> +++ b/drivers/platform/x86/intel/tpmi.c >>> @@ -215,8 +215,8 @@ static int tpmi_create_device(struct >>> intel_tpmi_info *tpmi_info, >>> >>> feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev), >>> GFP_KERNEL); >>> if (!feature_vsec_dev) { >>> - ret = -ENOMEM; >>> - goto free_res; >>> + kfree(res); >>> + return -ENOMEM; >>> } >>> >>> snprintf(feature_id_name, sizeof(feature_id_name), "tpmi- >>> %s", name); >>> @@ -242,17 +242,8 @@ static int tpmi_create_device(struct >>> intel_tpmi_info *tpmi_info, >>> * feature_vsec_dev memory is also freed as part of device >>> * delete. >>> */ >>> - ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- >>>> auxdev.dev, >>> - feature_vsec_dev, >>> feature_id_name); >>> - if (ret) >>> - goto free_res; >>> - >>> - return 0; >>> - >>> -free_res: >>> - kfree(res); >>> - >>> - return ret; >>> + return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- >>>> auxdev.dev, >>> + feature_vsec_dev, >>> feature_id_name); >>> } >>> >>> static int tpmi_create_devices(struct intel_tpmi_info *tpmi_info) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH linux-next v2 1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device() 2023-03-17 1:28 ` Dongliang Mu @ 2023-03-17 8:51 ` Hans de Goede 2023-03-17 10:23 ` srinivas pandruvada 2023-03-17 10:27 ` srinivas pandruvada 1 sibling, 1 reply; 18+ messages in thread From: Hans de Goede @ 2023-03-17 8:51 UTC (permalink / raw) To: Dongliang Mu, srinivas pandruvada, Mark Gross Cc: platform-driver-x86, linux-kernel Hi, On 3/17/23 02:28, Dongliang Mu wrote: > > On 2023/3/17 02:18, srinivas pandruvada wrote: >> Hi Hans, >> >> On Thu, 2023-03-16 at 15:25 +0100, Hans de Goede wrote: >>> Hi, >>> >>> On 3/9/23 05:01, Dongliang Mu wrote: >>>> The previous commit 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix >>>> double free reported by Smatch") incorrectly handle the >>>> deallocation of >>>> res variable. As shown in the comment, intel_vsec_add_aux handles >>>> all >>>> the deallocation of res and feature_vsec_dev. Therefore, kfree(res) >>>> can >>>> still cause double free if intel_vsec_add_aux returns error. >>>> >>>> Fix this by adjusting the error handling part in >>>> tpmi_create_device, >>>> following the function intel_vsec_add_dev. >>>> >>>> Fixes: 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free >>>> reported by Smatch") >>>> Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn> >> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> >> >>> IIRC then after this v2 was posted I still saw some comments on the >>> original v1 which was not posted on the list. Without the v1 comments >>> being on the list and this archived, I have lost track of what the >>> status of these patches is. >>> >>> Srinivas, can you let me know if I should merge these, or if more >>> changes are necessary ? >>> >>> From the off-list discussion of v1 I got the impression more changes >>> are necessary, but I'm not sure. >> I was looking for changes submitted by the following patch >> " >> [PATCH linux-next v2 3/3] drivers/platform/x86/intel: fix a memory leak >> in intel_vsec_add_aux >> " >> >> Since I was not copied on this, I was unaware. So I was requesting this >> change. >> >> Thanks, >> Srinivas > > Hi Srinivas and Hans, > > How about folding these three patches into one patch and resend a v3 patch? > > This will get all people together and avoid the previous embarrassing sitation. If I understand things correctly then patch 1/3 needs 3/3 to function correctly, right ? I would not fold them together, smaller patches are easier to review / understand, but maybe change the order and put patch 3/3 first? (so make it 1/3) ? I can even do that when applying if you agree that is the better order. Regards, Hans >>>> --- >>>> drivers/platform/x86/intel/tpmi.c | 17 ++++------------- >>>> 1 file changed, 4 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/drivers/platform/x86/intel/tpmi.c >>>> b/drivers/platform/x86/intel/tpmi.c >>>> index c999732b0f1e..882fe5e4763f 100644 >>>> --- a/drivers/platform/x86/intel/tpmi.c >>>> +++ b/drivers/platform/x86/intel/tpmi.c >>>> @@ -215,8 +215,8 @@ static int tpmi_create_device(struct >>>> intel_tpmi_info *tpmi_info, >>>> feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev), >>>> GFP_KERNEL); >>>> if (!feature_vsec_dev) { >>>> - ret = -ENOMEM; >>>> - goto free_res; >>>> + kfree(res); >>>> + return -ENOMEM; >>>> } >>>> snprintf(feature_id_name, sizeof(feature_id_name), "tpmi- >>>> %s", name); >>>> @@ -242,17 +242,8 @@ static int tpmi_create_device(struct >>>> intel_tpmi_info *tpmi_info, >>>> * feature_vsec_dev memory is also freed as part of device >>>> * delete. >>>> */ >>>> - ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- >>>>> auxdev.dev, >>>> - feature_vsec_dev, >>>> feature_id_name); >>>> - if (ret) >>>> - goto free_res; >>>> - >>>> - return 0; >>>> - >>>> -free_res: >>>> - kfree(res); >>>> - >>>> - return ret; >>>> + return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- >>>>> auxdev.dev, >>>> + feature_vsec_dev, >>>> feature_id_name); >>>> } >>>> static int tpmi_create_devices(struct intel_tpmi_info *tpmi_info) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH linux-next v2 1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device() 2023-03-17 8:51 ` Hans de Goede @ 2023-03-17 10:23 ` srinivas pandruvada 2023-03-17 11:57 ` Dongliang Mu 0 siblings, 1 reply; 18+ messages in thread From: srinivas pandruvada @ 2023-03-17 10:23 UTC (permalink / raw) To: Hans de Goede, Dongliang Mu, Mark Gross; +Cc: platform-driver-x86, linux-kernel Hi, ... ... > > > > Hi Srinivas and Hans, > > > > How about folding these three patches into one patch and resend a > > v3 patch? > > > > This will get all people together and avoid the previous > > embarrassing sitation. > > If I understand things correctly then patch 1/3 needs 3/3 to function > correctly, right ? > > I would not fold them together, smaller patches are easier to review > / understand, but maybe change the order and put patch 3/3 first? (so > make it 1/3) ? That should be correct order. The patch 3/3 should be the first. > > I can even do that when applying if you agree that is the better > order. Agree. Thanks, Srinivas > > Regards, > > Hans > > > > > > > > --- > > > > > drivers/platform/x86/intel/tpmi.c | 17 ++++------------- > > > > > 1 file changed, 4 insertions(+), 13 deletions(-) > > > > > > > > > > diff --git a/drivers/platform/x86/intel/tpmi.c > > > > > b/drivers/platform/x86/intel/tpmi.c > > > > > index c999732b0f1e..882fe5e4763f 100644 > > > > > --- a/drivers/platform/x86/intel/tpmi.c > > > > > +++ b/drivers/platform/x86/intel/tpmi.c > > > > > @@ -215,8 +215,8 @@ static int tpmi_create_device(struct > > > > > intel_tpmi_info *tpmi_info, > > > > > feature_vsec_dev = > > > > > kzalloc(sizeof(*feature_vsec_dev), > > > > > GFP_KERNEL); > > > > > if (!feature_vsec_dev) { > > > > > - ret = -ENOMEM; > > > > > - goto free_res; > > > > > + kfree(res); > > > > > + return -ENOMEM; > > > > > } > > > > > snprintf(feature_id_name, sizeof(feature_id_name), > > > > > "tpmi- > > > > > %s", name); > > > > > @@ -242,17 +242,8 @@ static int tpmi_create_device(struct > > > > > intel_tpmi_info *tpmi_info, > > > > > * feature_vsec_dev memory is also freed as part of > > > > > device > > > > > * delete. > > > > > */ > > > > > - ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- > > > > > > auxdev.dev, > > > > > - feature_vsec_dev, > > > > > feature_id_name); > > > > > - if (ret) > > > > > - goto free_res; > > > > > - > > > > > - return 0; > > > > > - > > > > > -free_res: > > > > > - kfree(res); > > > > > - > > > > > - return ret; > > > > > + return intel_vsec_add_aux(vsec_dev->pcidev, > > > > > &vsec_dev- > > > > > > auxdev.dev, > > > > > + feature_vsec_dev, > > > > > feature_id_name); > > > > > } > > > > > static int tpmi_create_devices(struct intel_tpmi_info > > > > > *tpmi_info) > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH linux-next v2 1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device() 2023-03-17 10:23 ` srinivas pandruvada @ 2023-03-17 11:57 ` Dongliang Mu 0 siblings, 0 replies; 18+ messages in thread From: Dongliang Mu @ 2023-03-17 11:57 UTC (permalink / raw) To: srinivas pandruvada, Hans de Goede, Mark Gross Cc: platform-driver-x86, linux-kernel On 2023/3/17 18:23, srinivas pandruvada wrote: > Hi, > > ... > ... > >>> Hi Srinivas and Hans, >>> >>> How about folding these three patches into one patch and resend a >>> v3 patch? >>> >>> This will get all people together and avoid the previous >>> embarrassing sitation. >> If I understand things correctly then patch 1/3 needs 3/3 to function >> correctly, right ? >> >> I would not fold them together, smaller patches are easier to review >> / understand, but maybe change the order and put patch 3/3 first? (so >> make it 1/3) ? > That should be correct order. The patch 3/3 should be the first. Oh, yeah. The memory leak fix 3/3 should be the first. This is more reasonable. BTW, I separated this memory leak fix due to that the kernel mainline is still vulnerable to this memory leak problem. > >> I can even do that when applying if you agree that is the better >> order. > Agree. > > Thanks, > Srinivas > >> Regards, >> >> Hans >> >> >> >>>>>> --- >>>>>> drivers/platform/x86/intel/tpmi.c | 17 ++++------------- >>>>>> 1 file changed, 4 insertions(+), 13 deletions(-) >>>>>> >>>>>> diff --git a/drivers/platform/x86/intel/tpmi.c >>>>>> b/drivers/platform/x86/intel/tpmi.c >>>>>> index c999732b0f1e..882fe5e4763f 100644 >>>>>> --- a/drivers/platform/x86/intel/tpmi.c >>>>>> +++ b/drivers/platform/x86/intel/tpmi.c >>>>>> @@ -215,8 +215,8 @@ static int tpmi_create_device(struct >>>>>> intel_tpmi_info *tpmi_info, >>>>>> feature_vsec_dev = >>>>>> kzalloc(sizeof(*feature_vsec_dev), >>>>>> GFP_KERNEL); >>>>>> if (!feature_vsec_dev) { >>>>>> - ret = -ENOMEM; >>>>>> - goto free_res; >>>>>> + kfree(res); >>>>>> + return -ENOMEM; >>>>>> } >>>>>> snprintf(feature_id_name, sizeof(feature_id_name), >>>>>> "tpmi- >>>>>> %s", name); >>>>>> @@ -242,17 +242,8 @@ static int tpmi_create_device(struct >>>>>> intel_tpmi_info *tpmi_info, >>>>>> * feature_vsec_dev memory is also freed as part of >>>>>> device >>>>>> * delete. >>>>>> */ >>>>>> - ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- >>>>>>> auxdev.dev, >>>>>> - feature_vsec_dev, >>>>>> feature_id_name); >>>>>> - if (ret) >>>>>> - goto free_res; >>>>>> - >>>>>> - return 0; >>>>>> - >>>>>> -free_res: >>>>>> - kfree(res); >>>>>> - >>>>>> - return ret; >>>>>> + return intel_vsec_add_aux(vsec_dev->pcidev, >>>>>> &vsec_dev- >>>>>>> auxdev.dev, >>>>>> + feature_vsec_dev, >>>>>> feature_id_name); >>>>>> } >>>>>> static int tpmi_create_devices(struct intel_tpmi_info >>>>>> *tpmi_info) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH linux-next v2 1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device() 2023-03-17 1:28 ` Dongliang Mu 2023-03-17 8:51 ` Hans de Goede @ 2023-03-17 10:27 ` srinivas pandruvada 2023-03-20 2:43 ` Dongliang Mu 1 sibling, 1 reply; 18+ messages in thread From: srinivas pandruvada @ 2023-03-17 10:27 UTC (permalink / raw) To: Dongliang Mu, Hans de Goede, Mark Gross; +Cc: platform-driver-x86, linux-kernel Hi Dongliang, > ... ... > Hi Srinivas and Hans, > > How about folding these three patches into one patch and resend a v3 > patch? > > This will get all people together and avoid the previous embarrassing > sitation. This is NOT an embarrassing situation. Thanks for finding and fixing the issue. Thanks, Srinivas > > Dongliang Mu > > > > > > Regards, > > > > > > Hans > > > > > > > > > > > > > > > > --- > > > > drivers/platform/x86/intel/tpmi.c | 17 ++++------------- > > > > 1 file changed, 4 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/drivers/platform/x86/intel/tpmi.c > > > > b/drivers/platform/x86/intel/tpmi.c > > > > index c999732b0f1e..882fe5e4763f 100644 > > > > --- a/drivers/platform/x86/intel/tpmi.c > > > > +++ b/drivers/platform/x86/intel/tpmi.c > > > > @@ -215,8 +215,8 @@ static int tpmi_create_device(struct > > > > intel_tpmi_info *tpmi_info, > > > > > > > > feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev), > > > > GFP_KERNEL); > > > > if (!feature_vsec_dev) { > > > > - ret = -ENOMEM; > > > > - goto free_res; > > > > + kfree(res); > > > > + return -ENOMEM; > > > > } > > > > > > > > snprintf(feature_id_name, sizeof(feature_id_name), > > > > "tpmi- > > > > %s", name); > > > > @@ -242,17 +242,8 @@ static int tpmi_create_device(struct > > > > intel_tpmi_info *tpmi_info, > > > > * feature_vsec_dev memory is also freed as part of > > > > device > > > > * delete. > > > > */ > > > > - ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- > > > > > auxdev.dev, > > > > - feature_vsec_dev, > > > > feature_id_name); > > > > - if (ret) > > > > - goto free_res; > > > > - > > > > - return 0; > > > > - > > > > -free_res: > > > > - kfree(res); > > > > - > > > > - return ret; > > > > + return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- > > > > > auxdev.dev, > > > > + feature_vsec_dev, > > > > feature_id_name); > > > > } > > > > > > > > static int tpmi_create_devices(struct intel_tpmi_info > > > > *tpmi_info) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH linux-next v2 1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device() 2023-03-17 10:27 ` srinivas pandruvada @ 2023-03-20 2:43 ` Dongliang Mu 2023-03-20 6:32 ` srinivas pandruvada 0 siblings, 1 reply; 18+ messages in thread From: Dongliang Mu @ 2023-03-20 2:43 UTC (permalink / raw) To: srinivas pandruvada, Hans de Goede, Mark Gross Cc: platform-driver-x86, linux-kernel On 2023/3/17 18:27, srinivas pandruvada wrote: > Hi Dongliang, > > ... > ... > > >> Hi Srinivas and Hans, >> >> How about folding these three patches into one patch and resend a v3 >> patch? >> >> This will get all people together and avoid the previous embarrassing >> sitation. > This is NOT an embarrassing situation. > Thanks for finding and fixing the issue. > > Thanks, > Srinivas > Hi Srinivas, Any conclusion about this patch set? >> Dongliang Mu >> >>>> Regards, >>>> >>>> Hans >>>> >>>> >>>> >>>> >>>>> --- >>>>> drivers/platform/x86/intel/tpmi.c | 17 ++++------------- >>>>> 1 file changed, 4 insertions(+), 13 deletions(-) >>>>> >>>>> diff --git a/drivers/platform/x86/intel/tpmi.c >>>>> b/drivers/platform/x86/intel/tpmi.c >>>>> index c999732b0f1e..882fe5e4763f 100644 >>>>> --- a/drivers/platform/x86/intel/tpmi.c >>>>> +++ b/drivers/platform/x86/intel/tpmi.c >>>>> @@ -215,8 +215,8 @@ static int tpmi_create_device(struct >>>>> intel_tpmi_info *tpmi_info, >>>>> >>>>> feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev), >>>>> GFP_KERNEL); >>>>> if (!feature_vsec_dev) { >>>>> - ret = -ENOMEM; >>>>> - goto free_res; >>>>> + kfree(res); >>>>> + return -ENOMEM; >>>>> } >>>>> >>>>> snprintf(feature_id_name, sizeof(feature_id_name), >>>>> "tpmi- >>>>> %s", name); >>>>> @@ -242,17 +242,8 @@ static int tpmi_create_device(struct >>>>> intel_tpmi_info *tpmi_info, >>>>> * feature_vsec_dev memory is also freed as part of >>>>> device >>>>> * delete. >>>>> */ >>>>> - ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- >>>>>> auxdev.dev, >>>>> - feature_vsec_dev, >>>>> feature_id_name); >>>>> - if (ret) >>>>> - goto free_res; >>>>> - >>>>> - return 0; >>>>> - >>>>> -free_res: >>>>> - kfree(res); >>>>> - >>>>> - return ret; >>>>> + return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- >>>>>> auxdev.dev, >>>>> + feature_vsec_dev, >>>>> feature_id_name); >>>>> } >>>>> >>>>> static int tpmi_create_devices(struct intel_tpmi_info >>>>> *tpmi_info) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH linux-next v2 1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device() 2023-03-20 2:43 ` Dongliang Mu @ 2023-03-20 6:32 ` srinivas pandruvada 0 siblings, 0 replies; 18+ messages in thread From: srinivas pandruvada @ 2023-03-20 6:32 UTC (permalink / raw) To: Dongliang Mu, Hans de Goede, Mark Gross; +Cc: platform-driver-x86, linux-kernel On Mon, 2023-03-20 at 10:43 +0800, Dongliang Mu wrote: > > On 2023/3/17 18:27, srinivas pandruvada wrote: > > Hi Dongliang, > > > > ... > > ... > > > > > > > Hi Srinivas and Hans, > > > > > > How about folding these three patches into one patch and resend a > > > v3 > > > patch? > > > > > > This will get all people together and avoid the previous > > > embarrassing > > > sitation. > > This is NOT an embarrassing situation. > > Thanks for finding and fixing the issue. > > > > Thanks, > > Srinivas > > > Hi Srinivas, > > Any conclusion about this patch set? Hans can reorder patches as he suggested and apply. Thanks, Srinivas > > > > Dongliang Mu > > > > > > > > Regards, > > > > > > > > > > Hans > > > > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > drivers/platform/x86/intel/tpmi.c | 17 ++++------------- > > > > > > 1 file changed, 4 insertions(+), 13 deletions(-) > > > > > > > > > > > > diff --git a/drivers/platform/x86/intel/tpmi.c > > > > > > b/drivers/platform/x86/intel/tpmi.c > > > > > > index c999732b0f1e..882fe5e4763f 100644 > > > > > > --- a/drivers/platform/x86/intel/tpmi.c > > > > > > +++ b/drivers/platform/x86/intel/tpmi.c > > > > > > @@ -215,8 +215,8 @@ static int tpmi_create_device(struct > > > > > > intel_tpmi_info *tpmi_info, > > > > > > > > > > > > feature_vsec_dev = > > > > > > kzalloc(sizeof(*feature_vsec_dev), > > > > > > GFP_KERNEL); > > > > > > if (!feature_vsec_dev) { > > > > > > - ret = -ENOMEM; > > > > > > - goto free_res; > > > > > > + kfree(res); > > > > > > + return -ENOMEM; > > > > > > } > > > > > > > > > > > > snprintf(feature_id_name, > > > > > > sizeof(feature_id_name), > > > > > > "tpmi- > > > > > > %s", name); > > > > > > @@ -242,17 +242,8 @@ static int tpmi_create_device(struct > > > > > > intel_tpmi_info *tpmi_info, > > > > > > * feature_vsec_dev memory is also freed as part > > > > > > of > > > > > > device > > > > > > * delete. > > > > > > */ > > > > > > - ret = intel_vsec_add_aux(vsec_dev->pcidev, > > > > > > &vsec_dev- > > > > > > > auxdev.dev, > > > > > > - feature_vsec_dev, > > > > > > feature_id_name); > > > > > > - if (ret) > > > > > > - goto free_res; > > > > > > - > > > > > > - return 0; > > > > > > - > > > > > > -free_res: > > > > > > - kfree(res); > > > > > > - > > > > > > - return ret; > > > > > > + return intel_vsec_add_aux(vsec_dev->pcidev, > > > > > > &vsec_dev- > > > > > > > auxdev.dev, > > > > > > + feature_vsec_dev, > > > > > > feature_id_name); > > > > > > } > > > > > > > > > > > > static int tpmi_create_devices(struct intel_tpmi_info > > > > > > *tpmi_info) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH linux-next v2 1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device() 2023-03-09 4:01 ` [PATCH linux-next v2 1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device() Dongliang Mu 2023-03-14 14:22 ` Dan Carpenter 2023-03-16 14:25 ` Hans de Goede @ 2023-03-20 10:32 ` Hans de Goede 2 siblings, 0 replies; 18+ messages in thread From: Hans de Goede @ 2023-03-20 10:32 UTC (permalink / raw) To: Dongliang Mu, Srinivas Pandruvada, Mark Gross Cc: platform-driver-x86, linux-kernel Hi, On 3/9/23 05:01, Dongliang Mu wrote: > The previous commit 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix > double free reported by Smatch") incorrectly handle the deallocation of > res variable. As shown in the comment, intel_vsec_add_aux handles all > the deallocation of res and feature_vsec_dev. Therefore, kfree(res) can > still cause double free if intel_vsec_add_aux returns error. > > Fix this by adjusting the error handling part in tpmi_create_device, > following the function intel_vsec_add_dev. > > Fixes: 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free reported by Smatch") > Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn> Note this patch causes the following compiler warnings: CC [M] drivers/platform/x86/intel/tpmi.o drivers/platform/x86/intel/tpmi.c: In function ‘tpmi_create_device’: drivers/platform/x86/intel/tpmi.c:206:13: warning: unused variable ‘ret’ [-Wunused-variable] 206 | int ret, i; | ^~~ I have fixed this and squashed the fix into the patch while applying it, so there is no need to send a new version: Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > --- > drivers/platform/x86/intel/tpmi.c | 17 ++++------------- > 1 file changed, 4 insertions(+), 13 deletions(-) > > diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c > index c999732b0f1e..882fe5e4763f 100644 > --- a/drivers/platform/x86/intel/tpmi.c > +++ b/drivers/platform/x86/intel/tpmi.c > @@ -215,8 +215,8 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info, > > feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev), GFP_KERNEL); > if (!feature_vsec_dev) { > - ret = -ENOMEM; > - goto free_res; > + kfree(res); > + return -ENOMEM; > } > > snprintf(feature_id_name, sizeof(feature_id_name), "tpmi-%s", name); > @@ -242,17 +242,8 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info, > * feature_vsec_dev memory is also freed as part of device > * delete. > */ > - ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev, > - feature_vsec_dev, feature_id_name); > - if (ret) > - goto free_res; > - > - return 0; > - > -free_res: > - kfree(res); > - > - return ret; > + return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev, > + feature_vsec_dev, feature_id_name); > } > > static int tpmi_create_devices(struct intel_tpmi_info *tpmi_info) ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH linux-next v2 2/3] drivers/platform/x86/intel/tpmi: revise the comment of intel_vsec_add_aux [not found] <20230309040107.534716-1-dzm91@hust.edu.cn> 2023-03-09 4:01 ` [PATCH linux-next v2 1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device() Dongliang Mu @ 2023-03-09 4:01 ` Dongliang Mu 2023-03-20 10:33 ` Hans de Goede 2023-03-09 4:01 ` [PATCH linux-next v2 3/3] drivers/platform/x86/intel: fix a memory leak in intel_vsec_add_aux Dongliang Mu 2 siblings, 1 reply; 18+ messages in thread From: Dongliang Mu @ 2023-03-09 4:01 UTC (permalink / raw) To: Srinivas Pandruvada, Hans de Goede, Mark Gross Cc: Dongliang Mu, platform-driver-x86, linux-kernel intel_vsec_add_aux() is resource managed including res and feature_vsec_dev memory. Fix this by revising the comment of intel_vsec_add_aux since res variable will also be freed in the intel_vsec_add_aux. Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn> --- drivers/platform/x86/intel/tpmi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c index 882fe5e4763f..036d0e0dba19 100644 --- a/drivers/platform/x86/intel/tpmi.c +++ b/drivers/platform/x86/intel/tpmi.c @@ -239,8 +239,8 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info, /* * intel_vsec_add_aux() is resource managed, no explicit * delete is required on error or on module unload. - * feature_vsec_dev memory is also freed as part of device - * delete. + * feature_vsec_dev and res memory are also freed as part of + * device deletion. */ return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev, feature_vsec_dev, feature_id_name); -- 2.39.2 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH linux-next v2 2/3] drivers/platform/x86/intel/tpmi: revise the comment of intel_vsec_add_aux 2023-03-09 4:01 ` [PATCH linux-next v2 2/3] drivers/platform/x86/intel/tpmi: revise the comment of intel_vsec_add_aux Dongliang Mu @ 2023-03-20 10:33 ` Hans de Goede 2023-03-20 10:57 ` Dongliang Mu 0 siblings, 1 reply; 18+ messages in thread From: Hans de Goede @ 2023-03-20 10:33 UTC (permalink / raw) To: Dongliang Mu, Srinivas Pandruvada, Mark Gross Cc: platform-driver-x86, linux-kernel Hi, On 3/9/23 05:01, Dongliang Mu wrote: > intel_vsec_add_aux() is resource managed including res and > feature_vsec_dev memory. > > Fix this by revising the comment of intel_vsec_add_aux since res variable > will also be freed in the intel_vsec_add_aux. > > Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn> Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > --- > drivers/platform/x86/intel/tpmi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c > index 882fe5e4763f..036d0e0dba19 100644 > --- a/drivers/platform/x86/intel/tpmi.c > +++ b/drivers/platform/x86/intel/tpmi.c > @@ -239,8 +239,8 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info, > /* > * intel_vsec_add_aux() is resource managed, no explicit > * delete is required on error or on module unload. > - * feature_vsec_dev memory is also freed as part of device > - * delete. > + * feature_vsec_dev and res memory are also freed as part of > + * device deletion. > */ > return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev, > feature_vsec_dev, feature_id_name); ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH linux-next v2 2/3] drivers/platform/x86/intel/tpmi: revise the comment of intel_vsec_add_aux 2023-03-20 10:33 ` Hans de Goede @ 2023-03-20 10:57 ` Dongliang Mu 2023-03-20 13:40 ` Hans de Goede 0 siblings, 1 reply; 18+ messages in thread From: Dongliang Mu @ 2023-03-20 10:57 UTC (permalink / raw) To: Hans de Goede, Srinivas Pandruvada, Mark Gross Cc: platform-driver-x86, linux-kernel On 2023/3/20 18:33, Hans de Goede wrote: > Hi, > > On 3/9/23 05:01, Dongliang Mu wrote: >> intel_vsec_add_aux() is resource managed including res and >> feature_vsec_dev memory. >> >> Fix this by revising the comment of intel_vsec_add_aux since res variable >> will also be freed in the intel_vsec_add_aux. >> >> Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn> > Thank you for your patch, I've applied this patch to my review-hans > branch: > https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans > > Note it will show up in my review-hans branch once I've pushed my > local branch there, which might take a while. > > Once I've run some tests on this branch the patches there will be > added to the platform-drivers-x86/for-next branch and eventually > will be included in the pdx86 pull-request to Linus for the next > merge-window. > > Regards, > > Hans Thanks, Hans. Shall I send the patch related to memory leak to the mainline? The mainline is also prone to this issue? > > > >> --- >> drivers/platform/x86/intel/tpmi.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c >> index 882fe5e4763f..036d0e0dba19 100644 >> --- a/drivers/platform/x86/intel/tpmi.c >> +++ b/drivers/platform/x86/intel/tpmi.c >> @@ -239,8 +239,8 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info, >> /* >> * intel_vsec_add_aux() is resource managed, no explicit >> * delete is required on error or on module unload. >> - * feature_vsec_dev memory is also freed as part of device >> - * delete. >> + * feature_vsec_dev and res memory are also freed as part of >> + * device deletion. >> */ >> return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev, >> feature_vsec_dev, feature_id_name); ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH linux-next v2 2/3] drivers/platform/x86/intel/tpmi: revise the comment of intel_vsec_add_aux 2023-03-20 10:57 ` Dongliang Mu @ 2023-03-20 13:40 ` Hans de Goede 0 siblings, 0 replies; 18+ messages in thread From: Hans de Goede @ 2023-03-20 13:40 UTC (permalink / raw) To: Dongliang Mu, Srinivas Pandruvada, Mark Gross Cc: platform-driver-x86, linux-kernel Hi, On 3/20/23 11:57, Dongliang Mu wrote: > > On 2023/3/20 18:33, Hans de Goede wrote: >> Hi, >> >> On 3/9/23 05:01, Dongliang Mu wrote: >>> intel_vsec_add_aux() is resource managed including res and >>> feature_vsec_dev memory. >>> >>> Fix this by revising the comment of intel_vsec_add_aux since res variable >>> will also be freed in the intel_vsec_add_aux. >>> >>> Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn> >> Thank you for your patch, I've applied this patch to my review-hans >> branch: >> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans >> >> Note it will show up in my review-hans branch once I've pushed my >> local branch there, which might take a while. >> >> Once I've run some tests on this branch the patches there will be >> added to the platform-drivers-x86/for-next branch and eventually >> will be included in the pdx86 pull-request to Linus for the next >> merge-window. >> >> Regards, >> >> Hans > > Thanks, Hans. > > Shall I send the patch related to memory leak to the mainline? The mainline is also prone to this issue? There is no need for this, I have just moved these from my review-hans branch to the fixes branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=fixes So these will be included in my next fixes pull-req for mainline. Regards, Hans >>> --- >>> drivers/platform/x86/intel/tpmi.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c >>> index 882fe5e4763f..036d0e0dba19 100644 >>> --- a/drivers/platform/x86/intel/tpmi.c >>> +++ b/drivers/platform/x86/intel/tpmi.c >>> @@ -239,8 +239,8 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info, >>> /* >>> * intel_vsec_add_aux() is resource managed, no explicit >>> * delete is required on error or on module unload. >>> - * feature_vsec_dev memory is also freed as part of device >>> - * delete. >>> + * feature_vsec_dev and res memory are also freed as part of >>> + * device deletion. >>> */ >>> return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev, >>> feature_vsec_dev, feature_id_name); > ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH linux-next v2 3/3] drivers/platform/x86/intel: fix a memory leak in intel_vsec_add_aux [not found] <20230309040107.534716-1-dzm91@hust.edu.cn> 2023-03-09 4:01 ` [PATCH linux-next v2 1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device() Dongliang Mu 2023-03-09 4:01 ` [PATCH linux-next v2 2/3] drivers/platform/x86/intel/tpmi: revise the comment of intel_vsec_add_aux Dongliang Mu @ 2023-03-09 4:01 ` Dongliang Mu 2023-03-20 10:31 ` Hans de Goede 2 siblings, 1 reply; 18+ messages in thread From: Dongliang Mu @ 2023-03-09 4:01 UTC (permalink / raw) To: David E. Box, Hans de Goede, Mark Gross Cc: Dongliang Mu, platform-driver-x86, linux-kernel The first error handling code in intel_vsec_add_aux misses the deallocation of intel_vsec_dev->resource. Fix this by adding kfree(intel_vsec_dev->resource) in the error handling code. Reviewed-by: David E. Box <david.e.box@linux.intel.com> Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn> --- drivers/platform/x86/intel/vsec.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/platform/x86/intel/vsec.c b/drivers/platform/x86/intel/vsec.c index 13decf36c6de..2311c16cb975 100644 --- a/drivers/platform/x86/intel/vsec.c +++ b/drivers/platform/x86/intel/vsec.c @@ -154,6 +154,7 @@ int intel_vsec_add_aux(struct pci_dev *pdev, struct device *parent, ret = ida_alloc(intel_vsec_dev->ida, GFP_KERNEL); mutex_unlock(&vsec_ida_lock); if (ret < 0) { + kfree(intel_vsec_dev->resource); kfree(intel_vsec_dev); return ret; } -- 2.39.2 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH linux-next v2 3/3] drivers/platform/x86/intel: fix a memory leak in intel_vsec_add_aux 2023-03-09 4:01 ` [PATCH linux-next v2 3/3] drivers/platform/x86/intel: fix a memory leak in intel_vsec_add_aux Dongliang Mu @ 2023-03-20 10:31 ` Hans de Goede 0 siblings, 0 replies; 18+ messages in thread From: Hans de Goede @ 2023-03-20 10:31 UTC (permalink / raw) To: Dongliang Mu, David E. Box, Mark Gross; +Cc: platform-driver-x86, linux-kernel Hi, On 3/9/23 05:01, Dongliang Mu wrote: > The first error handling code in intel_vsec_add_aux misses the > deallocation of intel_vsec_dev->resource. > > Fix this by adding kfree(intel_vsec_dev->resource) in the error handling > code. > > Reviewed-by: David E. Box <david.e.box@linux.intel.com> > Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn> Thank you for your patch, I've applied this patch (as first patch in the series) to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > --- > drivers/platform/x86/intel/vsec.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/platform/x86/intel/vsec.c b/drivers/platform/x86/intel/vsec.c > index 13decf36c6de..2311c16cb975 100644 > --- a/drivers/platform/x86/intel/vsec.c > +++ b/drivers/platform/x86/intel/vsec.c > @@ -154,6 +154,7 @@ int intel_vsec_add_aux(struct pci_dev *pdev, struct device *parent, > ret = ida_alloc(intel_vsec_dev->ida, GFP_KERNEL); > mutex_unlock(&vsec_ida_lock); > if (ret < 0) { > + kfree(intel_vsec_dev->resource); > kfree(intel_vsec_dev); > return ret; > } ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-03-20 13:41 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20230309040107.534716-1-dzm91@hust.edu.cn> 2023-03-09 4:01 ` [PATCH linux-next v2 1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device() Dongliang Mu 2023-03-14 14:22 ` Dan Carpenter 2023-03-16 14:25 ` Hans de Goede 2023-03-16 18:18 ` srinivas pandruvada 2023-03-17 1:28 ` Dongliang Mu 2023-03-17 8:51 ` Hans de Goede 2023-03-17 10:23 ` srinivas pandruvada 2023-03-17 11:57 ` Dongliang Mu 2023-03-17 10:27 ` srinivas pandruvada 2023-03-20 2:43 ` Dongliang Mu 2023-03-20 6:32 ` srinivas pandruvada 2023-03-20 10:32 ` Hans de Goede 2023-03-09 4:01 ` [PATCH linux-next v2 2/3] drivers/platform/x86/intel/tpmi: revise the comment of intel_vsec_add_aux Dongliang Mu 2023-03-20 10:33 ` Hans de Goede 2023-03-20 10:57 ` Dongliang Mu 2023-03-20 13:40 ` Hans de Goede 2023-03-09 4:01 ` [PATCH linux-next v2 3/3] drivers/platform/x86/intel: fix a memory leak in intel_vsec_add_aux Dongliang Mu 2023-03-20 10:31 ` Hans de Goede
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).