| Message ID | 20250402032657.1762800-1-lizhijian@fujitsu.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | RDMA/rxe: Fix null pointer dereference in ODP MR check | expand |
On Wed, April 2, 2025 12:27 PM Li Zhijian wrote: > > The blktests/rnbd reported a null pointer dereference as following. > Similar to the mxl5, introduce a is_odp_mr() to check if the odp Typo: mxl5 Can this be amended when merging? > is enabled in this mr. > Thank you for the fix! If mr type is other than IB_MR_TYPE_USER, the null pointer dereference can occur since mr->umem is not set in that case. The fix looks good. Reviewed-by: Daisuke Matsuda <matsuda-daisuke@fujitsu.com>
在 2025/4/2 5:26, Li Zhijian 写道: > The blktests/rnbd reported a null pointer dereference as following. > Similar to the mxl5, introduce a is_odp_mr() to check if the odp > is enabled in this mr. > > Workqueue: rxe_wq do_work [rdma_rxe] > RIP: 0010:rxe_mr_copy+0x57/0x210 [rdma_rxe] > Code: 7c 04 48 89 f3 48 89 d5 41 89 cf 45 89 c4 0f 84 dc 00 00 00 89 ca e8 f8 f8 ff ff 85 c0 0f 85 75 01 00 00 49 8b 86 f0 00 00 00 <f6> 40 28 02 0f 85 98 01 00 00 41 8b 46 78 41 8b 8e 10 01 00 00 8d > RSP: 0018:ffffa0aac02cfcf8 EFLAGS: 00010246 > RAX: 0000000000000000 RBX: ffff9079cd440024 RCX: 0000000000000000 > RDX: 000000000000003c RSI: ffff9079cd440060 RDI: ffff9079cd665600 > RBP: ffff9079c0e5e45a R08: 0000000000000000 R09: 0000000000000000 > R10: 000000003c000000 R11: 0000000000225510 R12: 0000000000000000 > R13: 0000000000000000 R14: ffff9079cd665600 R15: 000000000000003c > FS: 0000000000000000(0000) GS:ffff907ccfa80000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 0000000000000028 CR3: 0000000119498001 CR4: 00000000001726f0 > Call Trace: > <TASK> > ? __die_body+0x1e/0x60 > ? page_fault_oops+0x14f/0x4c0 > ? rxe_mr_copy+0x57/0x210 [rdma_rxe] > ? search_bpf_extables+0x5f/0x80 > ? exc_page_fault+0x7e/0x180 > ? asm_exc_page_fault+0x26/0x30 > ? rxe_mr_copy+0x57/0x210 [rdma_rxe] > ? rxe_mr_copy+0x48/0x210 [rdma_rxe] > ? rxe_pool_get_index+0x50/0x90 [rdma_rxe] > rxe_receiver+0x1d98/0x2530 [rdma_rxe] > ? psi_task_switch+0x1ff/0x250 > ? finish_task_switch+0x92/0x2d0 > ? __schedule+0xbdf/0x17c0 > do_task+0x65/0x1e0 [rdma_rxe] > process_scheduled_works+0xaa/0x3f0 > worker_thread+0x117/0x240 > > Fixes: d03fb5c6599e ("RDMA/rxe: Allow registering MRs for On-Demand Paging") > Signed-off-by: Li Zhijian <lizhijian@fujitsu.com> > --- > drivers/infiniband/sw/rxe/rxe_loc.h | 6 ++++++ > drivers/infiniband/sw/rxe/rxe_mr.c | 4 ++-- > drivers/infiniband/sw/rxe/rxe_resp.c | 4 ++-- > 3 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h > index feb386d98d1d..0bc3fbb6554f 100644 > --- a/drivers/infiniband/sw/rxe/rxe_loc.h > +++ b/drivers/infiniband/sw/rxe/rxe_loc.h > @@ -140,6 +140,12 @@ static inline int qp_mtu(struct rxe_qp *qp) > return IB_MTU_4096; > } > > +static inline bool is_odp_mr(struct rxe_mr *mr) Previously I once discussed with Bob Pearson about the function names. Perhaps it is better to rename is_odp_mr to rxe_is_odp_mr? Since sometimes we debug in rdma, with a lot of functions with the same name, it is difficult to recognize the modules that this function belongs to. Thus, in rxe module, it is better to add rxe_ prefix to the function name. But anyway, this commit is fine. Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev> Zhu Yanjun > +{ > + return IS_ENABLED(CONFIG_INFINIBAND_ON_DEMAND_PAGING) && mr->umem && > + mr->umem->is_odp; > +} > + > void free_rd_atomic_resource(struct resp_res *res); > > static inline void rxe_advance_resp_resource(struct rxe_qp *qp) > diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c > index 868d2f0b74e9..432d864c3ce9 100644 > --- a/drivers/infiniband/sw/rxe/rxe_mr.c > +++ b/drivers/infiniband/sw/rxe/rxe_mr.c > @@ -323,7 +323,7 @@ int rxe_mr_copy(struct rxe_mr *mr, u64 iova, void *addr, > return err; > } > > - if (mr->umem->is_odp) > + if (is_odp_mr(mr)) > return rxe_odp_mr_copy(mr, iova, addr, length, dir); > else > return rxe_mr_copy_xarray(mr, iova, addr, length, dir); > @@ -536,7 +536,7 @@ int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, u64 value) > u64 *va; > > /* ODP is not supported right now. WIP. */ > - if (mr->umem->is_odp) > + if (is_odp_mr(mr)) > return RESPST_ERR_UNSUPPORTED_OPCODE; > > /* See IBA oA19-28 */ > diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c > index 54ba9ee1acc5..5d9174e408db 100644 > --- a/drivers/infiniband/sw/rxe/rxe_resp.c > +++ b/drivers/infiniband/sw/rxe/rxe_resp.c > @@ -650,7 +650,7 @@ static enum resp_states process_flush(struct rxe_qp *qp, > struct resp_res *res = qp->resp.res; > > /* ODP is not supported right now. WIP. */ > - if (mr->umem->is_odp) > + if (is_odp_mr(mr)) > return RESPST_ERR_UNSUPPORTED_OPCODE; > > /* oA19-14, oA19-15 */ > @@ -706,7 +706,7 @@ static enum resp_states atomic_reply(struct rxe_qp *qp, > if (!res->replay) { > u64 iova = qp->resp.va + qp->resp.offset; > > - if (mr->umem->is_odp) > + if (is_odp_mr(mr)) > err = rxe_odp_atomic_op(mr, iova, pkt->opcode, > atmeth_comp(pkt), > atmeth_swap_add(pkt),
On 02/04/2025 16:58, Zhu Yanjun wrote: > > Previously I once discussed with Bob Pearson about the function names. This is a frequently raised question, yet I have not discovered a definitive coding style(Please let me know if you have). According to my understanding, the common practice I adhere to is as follows: - Functions utilized within a single file often do not require a prefix, as current situation. - If a function is to be used across multiple files, I believe a prefix is essential. Thanks Zhijian > Perhaps it is better to rename is_odp_mr to rxe_is_odp_mr? > > Since sometimes we debug in rdma, with a lot of functions with the same name, it is difficult to recognize the modules that this function belongs to. > > Thus, in rxe module, it is better to add rxe_ prefix to the function name. But anyway, this commit is fine.
在 2025/4/3 4:59, Zhijian Li (Fujitsu) 写道: > > > On 02/04/2025 16:58, Zhu Yanjun wrote: >> >> Previously I once discussed with Bob Pearson about the function names. > > This is a frequently raised question, yet I have not discovered a definitive > coding style(Please let me know if you have). According to my understanding, > the common practice I adhere to is as follows: > You have your own principals. In the linux rdma maillist, there is no such common principals. From the debug perspective, it is better to add rxe_ prefix to make debuf life easier. But if you follow your advice to choose not to add rxe_ prefix, it is also OK. After all, no such principals exist currently. Zhu Yanjun > - Functions utilized within a single file often do not require a prefix, as current situation. > - If a function is to be used across multiple files, I believe a prefix is essential. > > Thanks > Zhijian > >> Perhaps it is better to rename is_odp_mr to rxe_is_odp_mr? >> >> Since sometimes we debug in rdma, with a lot of functions with the same name, it is difficult to recognize the modules that this function belongs to. >> >> Thus, in rxe module, it is better to add rxe_ prefix to the function name. But anyway, this commit is fine.
diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h index feb386d98d1d..0bc3fbb6554f 100644 --- a/drivers/infiniband/sw/rxe/rxe_loc.h +++ b/drivers/infiniband/sw/rxe/rxe_loc.h @@ -140,6 +140,12 @@ static inline int qp_mtu(struct rxe_qp *qp) return IB_MTU_4096; } +static inline bool is_odp_mr(struct rxe_mr *mr) +{ + return IS_ENABLED(CONFIG_INFINIBAND_ON_DEMAND_PAGING) && mr->umem && + mr->umem->is_odp; +} + void free_rd_atomic_resource(struct resp_res *res); static inline void rxe_advance_resp_resource(struct rxe_qp *qp) diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c index 868d2f0b74e9..432d864c3ce9 100644 --- a/drivers/infiniband/sw/rxe/rxe_mr.c +++ b/drivers/infiniband/sw/rxe/rxe_mr.c @@ -323,7 +323,7 @@ int rxe_mr_copy(struct rxe_mr *mr, u64 iova, void *addr, return err; } - if (mr->umem->is_odp) + if (is_odp_mr(mr)) return rxe_odp_mr_copy(mr, iova, addr, length, dir); else return rxe_mr_copy_xarray(mr, iova, addr, length, dir); @@ -536,7 +536,7 @@ int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, u64 value) u64 *va; /* ODP is not supported right now. WIP. */ - if (mr->umem->is_odp) + if (is_odp_mr(mr)) return RESPST_ERR_UNSUPPORTED_OPCODE; /* See IBA oA19-28 */ diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c index 54ba9ee1acc5..5d9174e408db 100644 --- a/drivers/infiniband/sw/rxe/rxe_resp.c +++ b/drivers/infiniband/sw/rxe/rxe_resp.c @@ -650,7 +650,7 @@ static enum resp_states process_flush(struct rxe_qp *qp, struct resp_res *res = qp->resp.res; /* ODP is not supported right now. WIP. */ - if (mr->umem->is_odp) + if (is_odp_mr(mr)) return RESPST_ERR_UNSUPPORTED_OPCODE; /* oA19-14, oA19-15 */ @@ -706,7 +706,7 @@ static enum resp_states atomic_reply(struct rxe_qp *qp, if (!res->replay) { u64 iova = qp->resp.va + qp->resp.offset; - if (mr->umem->is_odp) + if (is_odp_mr(mr)) err = rxe_odp_atomic_op(mr, iova, pkt->opcode, atmeth_comp(pkt), atmeth_swap_add(pkt),
The blktests/rnbd reported a null pointer dereference as following. Similar to the mxl5, introduce a is_odp_mr() to check if the odp is enabled in this mr. Workqueue: rxe_wq do_work [rdma_rxe] RIP: 0010:rxe_mr_copy+0x57/0x210 [rdma_rxe] Code: 7c 04 48 89 f3 48 89 d5 41 89 cf 45 89 c4 0f 84 dc 00 00 00 89 ca e8 f8 f8 ff ff 85 c0 0f 85 75 01 00 00 49 8b 86 f0 00 00 00 <f6> 40 28 02 0f 85 98 01 00 00 41 8b 46 78 41 8b 8e 10 01 00 00 8d RSP: 0018:ffffa0aac02cfcf8 EFLAGS: 00010246 RAX: 0000000000000000 RBX: ffff9079cd440024 RCX: 0000000000000000 RDX: 000000000000003c RSI: ffff9079cd440060 RDI: ffff9079cd665600 RBP: ffff9079c0e5e45a R08: 0000000000000000 R09: 0000000000000000 R10: 000000003c000000 R11: 0000000000225510 R12: 0000000000000000 R13: 0000000000000000 R14: ffff9079cd665600 R15: 000000000000003c FS: 0000000000000000(0000) GS:ffff907ccfa80000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000000000028 CR3: 0000000119498001 CR4: 00000000001726f0 Call Trace: <TASK> ? __die_body+0x1e/0x60 ? page_fault_oops+0x14f/0x4c0 ? rxe_mr_copy+0x57/0x210 [rdma_rxe] ? search_bpf_extables+0x5f/0x80 ? exc_page_fault+0x7e/0x180 ? asm_exc_page_fault+0x26/0x30 ? rxe_mr_copy+0x57/0x210 [rdma_rxe] ? rxe_mr_copy+0x48/0x210 [rdma_rxe] ? rxe_pool_get_index+0x50/0x90 [rdma_rxe] rxe_receiver+0x1d98/0x2530 [rdma_rxe] ? psi_task_switch+0x1ff/0x250 ? finish_task_switch+0x92/0x2d0 ? __schedule+0xbdf/0x17c0 do_task+0x65/0x1e0 [rdma_rxe] process_scheduled_works+0xaa/0x3f0 worker_thread+0x117/0x240 Fixes: d03fb5c6599e ("RDMA/rxe: Allow registering MRs for On-Demand Paging") Signed-off-by: Li Zhijian <lizhijian@fujitsu.com> --- drivers/infiniband/sw/rxe/rxe_loc.h | 6 ++++++ drivers/infiniband/sw/rxe/rxe_mr.c | 4 ++-- drivers/infiniband/sw/rxe/rxe_resp.c | 4 ++-- 3 files changed, 10 insertions(+), 4 deletions(-)