| Message ID | 047b210eb3b3a2e26703d8b0570a0a017789c169.1669361183.git.william.xuanziyang@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | octeontx2-pf: Fix several bugs in exception paths | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 9 of 9 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 7 this patch: 7 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 7 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
Hello, On Fri, 2022-11-25 at 15:45 +0800, Ziyang Xuan wrote: > otx2_sq_free_sqbs() will be called twice when goto "err_free_nix_queues" > label in otx2_init_hw_resources(). The first calling is within > otx2_free_sq_res() at "err_free_nix_queues" label, and the second calling > is at later "err_free_sq_ptrs" label. > > In otx2_sq_free_sqbs(), If sq->sqb_ptrs[i] is not 0, the memory page it > points to will be freed, and sq->sqb_ptrs[i] do not be assigned 0 after > memory page be freed. If otx2_sq_free_sqbs() is called twice, the memory > page pointed by sq->sqb_ptrs[i] will be freeed twice. To fix the bug, > assign 0 to sq->sqb_ptrs[i] after memory page be freed. > > Fixes: caa2da34fd25 ("octeontx2-pf: Initialize and config queues") > Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> > --- > drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c > index 9e10e7471b88..5a25fe51d102 100644 > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c > @@ -1146,6 +1146,7 @@ void otx2_sq_free_sqbs(struct otx2_nic *pfvf) > DMA_FROM_DEVICE, > DMA_ATTR_SKIP_CPU_SYNC); > put_page(virt_to_page(phys_to_virt(pa))); > + sq->sqb_ptrs[sqb] = 0; The above looks not needed... > } > sq->sqb_count = 0; ... as this will prevent the next invocation of otx2_sq_free_sqbs() from traversing and freeing any sq->sqb_ptrs[] element. Cheers, Paolo > }
> Hello, > > On Fri, 2022-11-25 at 15:45 +0800, Ziyang Xuan wrote: >> otx2_sq_free_sqbs() will be called twice when goto "err_free_nix_queues" >> label in otx2_init_hw_resources(). The first calling is within >> otx2_free_sq_res() at "err_free_nix_queues" label, and the second calling >> is at later "err_free_sq_ptrs" label. >> >> In otx2_sq_free_sqbs(), If sq->sqb_ptrs[i] is not 0, the memory page it >> points to will be freed, and sq->sqb_ptrs[i] do not be assigned 0 after >> memory page be freed. If otx2_sq_free_sqbs() is called twice, the memory >> page pointed by sq->sqb_ptrs[i] will be freeed twice. To fix the bug, >> assign 0 to sq->sqb_ptrs[i] after memory page be freed. >> >> Fixes: caa2da34fd25 ("octeontx2-pf: Initialize and config queues") >> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> >> --- >> drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c >> index 9e10e7471b88..5a25fe51d102 100644 >> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c >> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c >> @@ -1146,6 +1146,7 @@ void otx2_sq_free_sqbs(struct otx2_nic *pfvf) >> DMA_FROM_DEVICE, >> DMA_ATTR_SKIP_CPU_SYNC); >> put_page(virt_to_page(phys_to_virt(pa))); >> + sq->sqb_ptrs[sqb] = 0; > > The above looks not needed... >> } >> sq->sqb_count = 0; > > ... as this will prevent the next invocation of otx2_sq_free_sqbs() > from traversing and freeing any sq->sqb_ptrs[] element. Yes, you are right. I did pay much attention to sq->sqb_ptrs[], and omitted the for loop condition. Thank you! > > Cheers, > > Paolo >> } > > > . >
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c index 9e10e7471b88..5a25fe51d102 100644 --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c @@ -1146,6 +1146,7 @@ void otx2_sq_free_sqbs(struct otx2_nic *pfvf) DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC); put_page(virt_to_page(phys_to_virt(pa))); + sq->sqb_ptrs[sqb] = 0; } sq->sqb_count = 0; }
otx2_sq_free_sqbs() will be called twice when goto "err_free_nix_queues" label in otx2_init_hw_resources(). The first calling is within otx2_free_sq_res() at "err_free_nix_queues" label, and the second calling is at later "err_free_sq_ptrs" label. In otx2_sq_free_sqbs(), If sq->sqb_ptrs[i] is not 0, the memory page it points to will be freed, and sq->sqb_ptrs[i] do not be assigned 0 after memory page be freed. If otx2_sq_free_sqbs() is called twice, the memory page pointed by sq->sqb_ptrs[i] will be freeed twice. To fix the bug, assign 0 to sq->sqb_ptrs[i] after memory page be freed. Fixes: caa2da34fd25 ("octeontx2-pf: Initialize and config queues") Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> --- drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c | 1 + 1 file changed, 1 insertion(+)