| Message ID | 20230109012651.3127529-1-shaozhengchao@huawei.com |
|---|---|
| State | Accepted |
| Commit | 4a9c793b939e0d87852c5bcae648313bf2bb94ee |
| Headers | show |
| Series | Bluetooth: hci_sync: fix memory leak in hci_update_adv_data() | expand |
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=709918 ---Test result--- Test Summary: CheckPatch PASS 0.92 seconds GitLint PASS 0.30 seconds SubjectPrefix PASS 0.09 seconds BuildKernel PASS 38.84 seconds CheckAllWarning PASS 41.58 seconds CheckSparse PASS 47.14 seconds CheckSmatch PASS 125.03 seconds BuildKernel32 PASS 36.81 seconds TestRunnerSetup PASS 530.67 seconds TestRunner_l2cap-tester PASS 18.93 seconds TestRunner_iso-tester PASS 20.55 seconds TestRunner_bnep-tester PASS 6.73 seconds TestRunner_mgmt-tester PASS 130.79 seconds TestRunner_rfcomm-tester PASS 10.78 seconds TestRunner_sco-tester PASS 9.96 seconds TestRunner_ioctl-tester PASS 12.30 seconds TestRunner_mesh-tester PASS 8.37 seconds TestRunner_smp-tester PASS 9.40 seconds TestRunner_userchan-tester PASS 6.94 seconds IncrementalBuild PASS 34.65 seconds --- Regards, Linux Bluetooth
Hi Zhengchao, On Sun, Jan 8, 2023 at 5:17 PM Zhengchao Shao <shaozhengchao@huawei.com> wrote: > > When hci_cmd_sync_queue() failed in hci_update_adv_data(), inst_ptr is > not freed, which will cause memory leak. Add release process to error > path. > > Fixes: 651cd3d65b0f ("Bluetooth: convert hci_update_adv_data to hci_sync") > Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> > --- > net/bluetooth/hci_sync.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > index 9e2d7e4b850c..1485501bd72f 100644 > --- a/net/bluetooth/hci_sync.c > +++ b/net/bluetooth/hci_sync.c > @@ -6197,10 +6197,15 @@ static int _update_adv_data_sync(struct hci_dev *hdev, void *data) > int hci_update_adv_data(struct hci_dev *hdev, u8 instance) > { > u8 *inst_ptr = kmalloc(1, GFP_KERNEL); > + int ret; > > if (!inst_ptr) > return -ENOMEM; > > *inst_ptr = instance; > - return hci_cmd_sync_queue(hdev, _update_adv_data_sync, inst_ptr, NULL); > + ret = hci_cmd_sync_queue(hdev, _update_adv_data_sync, inst_ptr, NULL); > + if (ret) > + kfree(inst_ptr); > + > + return ret; > } While this is correct I do wonder why we haven't used ERR_PTR/PTR_ERR like we did with the likes of abort_conn_sync, that way we don't have to allocate anything which makes things a lot simpler. > -- > 2.34.1 >
On 2023/1/10 5:11, Luiz Augusto von Dentz wrote: > Hi Zhengchao, > > On Sun, Jan 8, 2023 at 5:17 PM Zhengchao Shao <shaozhengchao@huawei.com> wrote: >> >> When hci_cmd_sync_queue() failed in hci_update_adv_data(), inst_ptr is >> not freed, which will cause memory leak. Add release process to error >> path. >> >> Fixes: 651cd3d65b0f ("Bluetooth: convert hci_update_adv_data to hci_sync") >> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> >> --- >> net/bluetooth/hci_sync.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c >> index 9e2d7e4b850c..1485501bd72f 100644 >> --- a/net/bluetooth/hci_sync.c >> +++ b/net/bluetooth/hci_sync.c >> @@ -6197,10 +6197,15 @@ static int _update_adv_data_sync(struct hci_dev *hdev, void *data) >> int hci_update_adv_data(struct hci_dev *hdev, u8 instance) >> { >> u8 *inst_ptr = kmalloc(1, GFP_KERNEL); >> + int ret; >> >> if (!inst_ptr) >> return -ENOMEM; >> >> *inst_ptr = instance; >> - return hci_cmd_sync_queue(hdev, _update_adv_data_sync, inst_ptr, NULL); >> + ret = hci_cmd_sync_queue(hdev, _update_adv_data_sync, inst_ptr, NULL); >> + if (ret) >> + kfree(inst_ptr); >> + >> + return ret; >> } > > While this is correct I do wonder why we haven't used ERR_PTR/PTR_ERR > like we did with the likes of abort_conn_sync, that way we don't have > to allocate anything which makes things a lot simpler. > Hi Luiz: Thank you for your advice. I think it is better to use ERR_PTR/PTR_ERR to replace allocation. I will send V2. Zhengchao Shao >> -- >> 2.34.1 >> > >
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c index 9e2d7e4b850c..1485501bd72f 100644 --- a/net/bluetooth/hci_sync.c +++ b/net/bluetooth/hci_sync.c @@ -6197,10 +6197,15 @@ static int _update_adv_data_sync(struct hci_dev *hdev, void *data) int hci_update_adv_data(struct hci_dev *hdev, u8 instance) { u8 *inst_ptr = kmalloc(1, GFP_KERNEL); + int ret; if (!inst_ptr) return -ENOMEM; *inst_ptr = instance; - return hci_cmd_sync_queue(hdev, _update_adv_data_sync, inst_ptr, NULL); + ret = hci_cmd_sync_queue(hdev, _update_adv_data_sync, inst_ptr, NULL); + if (ret) + kfree(inst_ptr); + + return ret; }
When hci_cmd_sync_queue() failed in hci_update_adv_data(), inst_ptr is not freed, which will cause memory leak. Add release process to error path. Fixes: 651cd3d65b0f ("Bluetooth: convert hci_update_adv_data to hci_sync") Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> --- net/bluetooth/hci_sync.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)