| Message ID | 20190819100526.13788-1-geert@linux-m68k.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RDMA/siw: Fix compiler warnings on 32-bit due to u64/pointer abuse | expand |
On Mon, Aug 19, 2019 at 12:05:26PM +0200, Geert Uytterhoeven wrote: > When compiling on 32-bit: > > drivers/infiniband/sw/siw/siw_cq.c:76:20: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > drivers/infiniband/sw/siw/siw_qp.c:952:28: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] > drivers/infiniband/sw/siw/siw_qp_tx.c:53:10: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > drivers/infiniband/sw/siw/siw_qp_tx.c:59:11: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > drivers/infiniband/sw/siw/siw_qp_tx.c:59:26: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > drivers/infiniband/sw/siw/siw_qp_tx.c:61:23: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > drivers/infiniband/sw/siw/siw_qp_tx.c:62:9: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > drivers/infiniband/sw/siw/siw_qp_tx.c:82:12: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > drivers/infiniband/sw/siw/siw_qp_tx.c:87:12: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > drivers/infiniband/sw/siw/siw_qp_tx.c:101:12: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > drivers/infiniband/sw/siw/siw_qp_tx.c:169:29: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] > drivers/infiniband/sw/siw/siw_qp_tx.c:192:29: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] > drivers/infiniband/sw/siw/siw_qp_tx.c:204:29: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] > drivers/infiniband/sw/siw/siw_qp_tx.c:219:29: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] > drivers/infiniband/sw/siw/siw_qp_tx.c:476:24: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > drivers/infiniband/sw/siw/siw_qp_tx.c:535:7: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > drivers/infiniband/sw/siw/siw_qp_tx.c:832:29: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] > drivers/infiniband/sw/siw/siw_qp_tx.c:927:26: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > drivers/infiniband/sw/siw/siw_qp_rx.c:43:5: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > drivers/infiniband/sw/siw/siw_qp_rx.c:43:24: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > drivers/infiniband/sw/siw/siw_qp_rx.c:141:23: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > drivers/infiniband/sw/siw/siw_qp_rx.c:488:6: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > drivers/infiniband/sw/siw/siw_qp_rx.c:601:5: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > drivers/infiniband/sw/siw/siw_qp_rx.c:844:24: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > drivers/infiniband/sw/siw/siw_verbs.c:665:22: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] > drivers/infiniband/sw/siw/siw_verbs.c:828:19: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] > drivers/infiniband/sw/siw/siw_verbs.c:846:32: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > > Fix this by applying the following rules: > 1. When printing a u64, the %llx format specififer should be used, > instead of casting to a pointer, and printing the latter. > 2. When assigning a pointer to a u64, the pointer should be cast to > uintptr_t, not u64, > 3. When casting from u64 to pointer, an intermediate cast to uintptr_t > should be added, > > Fixes: 2c8ccb37b08fe364 ("RDMA/siw: Change CQ flags from 64->32 bits") > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > The issues predate the commit mentioned above, but didn't become visible > before. > > The Right Thing(TM) would be to get rid of all this casting, and use > proper types instead. > This would involve teaching the siw people that a kernel virtual address > is not called a physical address, and should not use u64. > drivers/infiniband/sw/siw/siw_cq.c | 5 ++-- > drivers/infiniband/sw/siw/siw_qp.c | 2 +- > drivers/infiniband/sw/siw/siw_qp_rx.c | 16 +++++++------ > drivers/infiniband/sw/siw/siw_qp_tx.c | 34 ++++++++++++++------------- > drivers/infiniband/sw/siw/siw_verbs.c | 8 +++---- > 5 files changed, 35 insertions(+), 30 deletions(-) > > diff --git a/drivers/infiniband/sw/siw/siw_cq.c b/drivers/infiniband/sw/siw/siw_cq.c > index e381ae9b7d62498e..f4ec26eeb9df62bf 100644 > +++ b/drivers/infiniband/sw/siw/siw_cq.c > @@ -71,9 +71,10 @@ int siw_reap_cqe(struct siw_cq *cq, struct ib_wc *wc) > wc->wc_flags = IB_WC_WITH_INVALIDATE; > } > wc->qp = cqe->base_qp; > - siw_dbg_cq(cq, "idx %u, type %d, flags %2x, id 0x%p\n", > + siw_dbg_cq(cq, > + "idx %u, type %d, flags %2x, id 0x%llx\n", > cq->cq_get % cq->num_cqe, cqe->opcode, > - cqe->flags, (void *)cqe->id); > + cqe->flags, cqe->id); If the value is really a kernel pointer, then it ought to be printed with %p. We have been getting demanding on this point lately in RDMA to enforce the ability to keep kernel pointers secret. > - wqe->sqe.sge[0].laddr = (u64)&wqe->sqe.sge[1]; > + wqe->sqe.sge[0].laddr = (uintptr_t)&wqe->sqe.sge[1]; [..] > rv = siw_rx_kva(srx, > - (void *)(sge->laddr + frx->sge_off), > + (void *)(uintptr_t)(sge->laddr + frx->sge_off), > sge_bytes); Bernard, this is nonsense, what is going on here with sge->laddr that it can't be a void *? Jason
-----"Jason Gunthorpe" <jgg@ziepe.ca> wrote: ----- >To: "Geert Uytterhoeven" <geert@linux-m68k.org> >From: "Jason Gunthorpe" <jgg@ziepe.ca> >Date: 08/19/2019 02:25PM >Cc: "Bernard Metzler" <bmt@zurich.ibm.com>, "Doug Ledford" ><dledford@redhat.com>, linux-rdma@vger.kernel.org, >linux-kernel@vger.kernel.org >Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Fix compiler warnings on >32-bit due to u64/pointer abuse > >On Mon, Aug 19, 2019 at 12:05:26PM +0200, Geert Uytterhoeven wrote: >> When compiling on 32-bit: >> >> drivers/infiniband/sw/siw/siw_cq.c:76:20: warning: cast to >pointer from integer of different size [-Wint-to-pointer-cast] >> drivers/infiniband/sw/siw/siw_qp.c:952:28: warning: cast from >pointer to integer of different size [-Wpointer-to-int-cast] >> drivers/infiniband/sw/siw/siw_qp_tx.c:53:10: warning: cast to >pointer from integer of different size [-Wint-to-pointer-cast] >> drivers/infiniband/sw/siw/siw_qp_tx.c:59:11: warning: cast to >pointer from integer of different size [-Wint-to-pointer-cast] >> drivers/infiniband/sw/siw/siw_qp_tx.c:59:26: warning: cast to >pointer from integer of different size [-Wint-to-pointer-cast] >> drivers/infiniband/sw/siw/siw_qp_tx.c:61:23: warning: cast to >pointer from integer of different size [-Wint-to-pointer-cast] >> drivers/infiniband/sw/siw/siw_qp_tx.c:62:9: warning: cast to >pointer from integer of different size [-Wint-to-pointer-cast] >> drivers/infiniband/sw/siw/siw_qp_tx.c:82:12: warning: cast to >pointer from integer of different size [-Wint-to-pointer-cast] >> drivers/infiniband/sw/siw/siw_qp_tx.c:87:12: warning: cast to >pointer from integer of different size [-Wint-to-pointer-cast] >> drivers/infiniband/sw/siw/siw_qp_tx.c:101:12: warning: cast to >pointer from integer of different size [-Wint-to-pointer-cast] >> drivers/infiniband/sw/siw/siw_qp_tx.c:169:29: warning: cast >from pointer to integer of different size [-Wpointer-to-int-cast] >> drivers/infiniband/sw/siw/siw_qp_tx.c:192:29: warning: cast >from pointer to integer of different size [-Wpointer-to-int-cast] >> drivers/infiniband/sw/siw/siw_qp_tx.c:204:29: warning: cast >from pointer to integer of different size [-Wpointer-to-int-cast] >> drivers/infiniband/sw/siw/siw_qp_tx.c:219:29: warning: cast >from pointer to integer of different size [-Wpointer-to-int-cast] >> drivers/infiniband/sw/siw/siw_qp_tx.c:476:24: warning: cast to >pointer from integer of different size [-Wint-to-pointer-cast] >> drivers/infiniband/sw/siw/siw_qp_tx.c:535:7: warning: cast to >pointer from integer of different size [-Wint-to-pointer-cast] >> drivers/infiniband/sw/siw/siw_qp_tx.c:832:29: warning: cast >from pointer to integer of different size [-Wpointer-to-int-cast] >> drivers/infiniband/sw/siw/siw_qp_tx.c:927:26: warning: cast to >pointer from integer of different size [-Wint-to-pointer-cast] >> drivers/infiniband/sw/siw/siw_qp_rx.c:43:5: warning: cast to >pointer from integer of different size [-Wint-to-pointer-cast] >> drivers/infiniband/sw/siw/siw_qp_rx.c:43:24: warning: cast to >pointer from integer of different size [-Wint-to-pointer-cast] >> drivers/infiniband/sw/siw/siw_qp_rx.c:141:23: warning: cast to >pointer from integer of different size [-Wint-to-pointer-cast] >> drivers/infiniband/sw/siw/siw_qp_rx.c:488:6: warning: cast to >pointer from integer of different size [-Wint-to-pointer-cast] >> drivers/infiniband/sw/siw/siw_qp_rx.c:601:5: warning: cast to >pointer from integer of different size [-Wint-to-pointer-cast] >> drivers/infiniband/sw/siw/siw_qp_rx.c:844:24: warning: cast to >pointer from integer of different size [-Wint-to-pointer-cast] >> drivers/infiniband/sw/siw/siw_verbs.c:665:22: warning: cast >from pointer to integer of different size [-Wpointer-to-int-cast] >> drivers/infiniband/sw/siw/siw_verbs.c:828:19: warning: cast >from pointer to integer of different size [-Wpointer-to-int-cast] >> drivers/infiniband/sw/siw/siw_verbs.c:846:32: warning: cast to >pointer from integer of different size [-Wint-to-pointer-cast] >> >> Fix this by applying the following rules: >> 1. When printing a u64, the %llx format specififer should be >used, >> instead of casting to a pointer, and printing the latter. >> 2. When assigning a pointer to a u64, the pointer should be cast >to >> uintptr_t, not u64, >> 3. When casting from u64 to pointer, an intermediate cast to >uintptr_t >> should be added, >> >> Fixes: 2c8ccb37b08fe364 ("RDMA/siw: Change CQ flags from 64->32 >bits") >> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> >> The issues predate the commit mentioned above, but didn't become >visible >> before. >> >> The Right Thing(TM) would be to get rid of all this casting, and >use >> proper types instead. >> This would involve teaching the siw people that a kernel virtual >address >> is not called a physical address, and should not use u64. >> drivers/infiniband/sw/siw/siw_cq.c | 5 ++-- >> drivers/infiniband/sw/siw/siw_qp.c | 2 +- >> drivers/infiniband/sw/siw/siw_qp_rx.c | 16 +++++++------ >> drivers/infiniband/sw/siw/siw_qp_tx.c | 34 >++++++++++++++------------- >> drivers/infiniband/sw/siw/siw_verbs.c | 8 +++---- >> 5 files changed, 35 insertions(+), 30 deletions(-) >> >> diff --git a/drivers/infiniband/sw/siw/siw_cq.c >b/drivers/infiniband/sw/siw/siw_cq.c >> index e381ae9b7d62498e..f4ec26eeb9df62bf 100644 >> +++ b/drivers/infiniband/sw/siw/siw_cq.c >> @@ -71,9 +71,10 @@ int siw_reap_cqe(struct siw_cq *cq, struct ib_wc >*wc) >> wc->wc_flags = IB_WC_WITH_INVALIDATE; >> } >> wc->qp = cqe->base_qp; >> - siw_dbg_cq(cq, "idx %u, type %d, flags %2x, id 0x%p\n", >> + siw_dbg_cq(cq, >> + "idx %u, type %d, flags %2x, id 0x%llx\n", >> cq->cq_get % cq->num_cqe, cqe->opcode, >> - cqe->flags, (void *)cqe->id); >> + cqe->flags, cqe->id); > >If the value is really a kernel pointer, then it ought to be printed >with %p. We have been getting demanding on this point lately in RDMA >to enforce the ability to keep kernel pointers secret. > >> - wqe->sqe.sge[0].laddr = (u64)&wqe->sqe.sge[1]; >> + wqe->sqe.sge[0].laddr = (uintptr_t)&wqe->sqe.sge[1]; > >[..] > >> rv = siw_rx_kva(srx, >> - (void *)(sge->laddr + frx->sge_off), >> + (void *)(uintptr_t)(sge->laddr + frx->sge_off), >> sge_bytes); > >Bernard, this is nonsense, what is going on here with sge->laddr that >it can't be a void *? > siw_sge is defined in siw-abi.h. We make the address u64 to keep the ABI arch independent. Thanks and best regards, Bernard.
On Mon, Aug 19, 2019 at 01:36:11PM +0000, Bernard Metzler wrote: > >If the value is really a kernel pointer, then it ought to be printed > >with %p. We have been getting demanding on this point lately in RDMA > >to enforce the ability to keep kernel pointers secret. > > > >> - wqe->sqe.sge[0].laddr = (u64)&wqe->sqe.sge[1]; > >> + wqe->sqe.sge[0].laddr = (uintptr_t)&wqe->sqe.sge[1]; > > > >[..] > > > >> rv = siw_rx_kva(srx, > >> - (void *)(sge->laddr + frx->sge_off), > >> + (void *)(uintptr_t)(sge->laddr + frx->sge_off), > >> sge_bytes); > > > >Bernard, this is nonsense, what is going on here with sge->laddr that > >it can't be a void *? > > > siw_sge is defined in siw-abi.h. We make the address u64 to keep the ABI > arch independent. Eh? How does the siw-abi.h store a kernel pointer? Sounds like kernel and user types are being mixed. Jason
-----"Jason Gunthorpe" <jgg@ziepe.ca> wrote: ----- >To: "Bernard Metzler" <BMT@zurich.ibm.com> >From: "Jason Gunthorpe" <jgg@ziepe.ca> >Date: 08/19/2019 03:52PM >Cc: "Geert Uytterhoeven" <geert@linux-m68k.org>, "Doug Ledford" ><dledford@redhat.com>, linux-rdma@vger.kernel.org, >linux-kernel@vger.kernel.org >Subject: [EXTERNAL] Re: Re: [PATCH] RDMA/siw: Fix compiler warnings >on 32-bit due to u64/pointer abuse > >On Mon, Aug 19, 2019 at 01:36:11PM +0000, Bernard Metzler wrote: >> >If the value is really a kernel pointer, then it ought to be >printed >> >with %p. We have been getting demanding on this point lately in >RDMA >> >to enforce the ability to keep kernel pointers secret. >> > >> >> - wqe->sqe.sge[0].laddr = (u64)&wqe->sqe.sge[1]; >> >> + wqe->sqe.sge[0].laddr = (uintptr_t)&wqe->sqe.sge[1]; >> > >> >[..] >> > >> >> rv = siw_rx_kva(srx, >> >> - (void *)(sge->laddr + frx->sge_off), >> >> + (void *)(uintptr_t)(sge->laddr + frx->sge_off), >> >> sge_bytes); >> > >> >Bernard, this is nonsense, what is going on here with sge->laddr >that >> >it can't be a void *? >> > >> siw_sge is defined in siw-abi.h. We make the address u64 to keep >the ABI >> arch independent. > >Eh? How does the siw-abi.h store a kernel pointer? Sounds like kernel >and user types are being mixed. > siw-abi.h defines the work queue elements of a siw send queue. For user land, the send queue is mmapped. Kernel or user land clients write to its send queue when posting work (SGE: buffer address, length, local key). Thanks, Bernard.
On Mon, Aug 19, 2019 at 02:15:36PM +0000, Bernard Metzler wrote: > > >To: "Bernard Metzler" <BMT@zurich.ibm.com> > >From: "Jason Gunthorpe" <jgg@ziepe.ca> > >Date: 08/19/2019 03:52PM > >Cc: "Geert Uytterhoeven" <geert@linux-m68k.org>, "Doug Ledford" > ><dledford@redhat.com>, linux-rdma@vger.kernel.org, > >linux-kernel@vger.kernel.org > >Subject: [EXTERNAL] Re: Re: [PATCH] RDMA/siw: Fix compiler warnings > >on 32-bit due to u64/pointer abuse > > > >On Mon, Aug 19, 2019 at 01:36:11PM +0000, Bernard Metzler wrote: > >> >If the value is really a kernel pointer, then it ought to be > >printed > >> >with %p. We have been getting demanding on this point lately in > >RDMA > >> >to enforce the ability to keep kernel pointers secret. > >> > > >> >> - wqe->sqe.sge[0].laddr = (u64)&wqe->sqe.sge[1]; > >> >> + wqe->sqe.sge[0].laddr = (uintptr_t)&wqe->sqe.sge[1]; > >> > > >> >[..] > >> > > >> >> rv = siw_rx_kva(srx, > >> >> - (void *)(sge->laddr + frx->sge_off), > >> >> + (void *)(uintptr_t)(sge->laddr + frx->sge_off), > >> >> sge_bytes); > >> > > >> >Bernard, this is nonsense, what is going on here with sge->laddr > >that > >> >it can't be a void *? > >> > > >> siw_sge is defined in siw-abi.h. We make the address u64 to keep > >the ABI > >> arch independent. > > > >Eh? How does the siw-abi.h store a kernel pointer? Sounds like kernel > >and user types are being mixed. > > > > siw-abi.h defines the work queue elements of a siw send queue. > For user land, the send queue is mmapped. Kernel or user land > clients write to its send queue when posting work > (SGE: buffer address, length, local key). Should have different types.. Don't want to accidently mix a laddr under user control with one under kernel control. Jason
-----"Jason Gunthorpe" <jgg@ziepe.ca> wrote: ----- >To: "Bernard Metzler" <BMT@zurich.ibm.com> >From: "Jason Gunthorpe" <jgg@ziepe.ca> >Date: 08/19/2019 04:19PM >Cc: "Geert Uytterhoeven" <geert@linux-m68k.org>, "Doug Ledford" ><dledford@redhat.com>, linux-rdma@vger.kernel.org, >linux-kernel@vger.kernel.org >Subject: [EXTERNAL] Re: Re: Re: [PATCH] RDMA/siw: Fix compiler >warnings on 32-bit due to u64/pointer abuse > >On Mon, Aug 19, 2019 at 02:15:36PM +0000, Bernard Metzler wrote: >> >> >To: "Bernard Metzler" <BMT@zurich.ibm.com> >> >From: "Jason Gunthorpe" <jgg@ziepe.ca> >> >Date: 08/19/2019 03:52PM >> >Cc: "Geert Uytterhoeven" <geert@linux-m68k.org>, "Doug Ledford" >> ><dledford@redhat.com>, linux-rdma@vger.kernel.org, >> >linux-kernel@vger.kernel.org >> >Subject: [EXTERNAL] Re: Re: [PATCH] RDMA/siw: Fix compiler >warnings >> >on 32-bit due to u64/pointer abuse >> > >> >On Mon, Aug 19, 2019 at 01:36:11PM +0000, Bernard Metzler wrote: >> >> >If the value is really a kernel pointer, then it ought to be >> >printed >> >> >with %p. We have been getting demanding on this point lately in >> >RDMA >> >> >to enforce the ability to keep kernel pointers secret. >> >> > >> >> >> - wqe->sqe.sge[0].laddr = (u64)&wqe->sqe.sge[1]; >> >> >> + wqe->sqe.sge[0].laddr = (uintptr_t)&wqe->sqe.sge[1]; >> >> > >> >> >[..] >> >> > >> >> >> rv = siw_rx_kva(srx, >> >> >> - (void *)(sge->laddr + frx->sge_off), >> >> >> + (void *)(uintptr_t)(sge->laddr + frx->sge_off), >> >> >> sge_bytes); >> >> > >> >> >Bernard, this is nonsense, what is going on here with >sge->laddr >> >that >> >> >it can't be a void *? >> >> > >> >> siw_sge is defined in siw-abi.h. We make the address u64 to keep >> >the ABI >> >> arch independent. >> > >> >Eh? How does the siw-abi.h store a kernel pointer? Sounds like >kernel >> >and user types are being mixed. >> > >> >> siw-abi.h defines the work queue elements of a siw send queue. >> For user land, the send queue is mmapped. Kernel or user land >> clients write to its send queue when posting work >> (SGE: buffer address, length, local key). > >Should have different types.. Don't want to accidently mix a laddr >under user control with one under kernel control. > Well we have an unsigned 64bit for both user and kernel application buffer addresses throughout the rdma stack, and we have it on the wire for all transports as well. Why do we want to have it differently for the siw driver? Thanks and best regards Bernard.
On Mon, Aug 19, 2019 at 02:52:13PM +0000, Bernard Metzler wrote: > > >To: "Bernard Metzler" <BMT@zurich.ibm.com> > >From: "Jason Gunthorpe" <jgg@ziepe.ca> > >Date: 08/19/2019 04:19PM > >Cc: "Geert Uytterhoeven" <geert@linux-m68k.org>, "Doug Ledford" > ><dledford@redhat.com>, linux-rdma@vger.kernel.org, > >linux-kernel@vger.kernel.org > >Subject: [EXTERNAL] Re: Re: Re: [PATCH] RDMA/siw: Fix compiler > >warnings on 32-bit due to u64/pointer abuse > > > >On Mon, Aug 19, 2019 at 02:15:36PM +0000, Bernard Metzler wrote: > >> > >> >To: "Bernard Metzler" <BMT@zurich.ibm.com> > >> >From: "Jason Gunthorpe" <jgg@ziepe.ca> > >> >Date: 08/19/2019 03:52PM > >> >Cc: "Geert Uytterhoeven" <geert@linux-m68k.org>, "Doug Ledford" > >> ><dledford@redhat.com>, linux-rdma@vger.kernel.org, > >> >linux-kernel@vger.kernel.org > >> >Subject: [EXTERNAL] Re: Re: [PATCH] RDMA/siw: Fix compiler > >warnings > >> >on 32-bit due to u64/pointer abuse > >> > > >> >On Mon, Aug 19, 2019 at 01:36:11PM +0000, Bernard Metzler wrote: > >> >> >If the value is really a kernel pointer, then it ought to be > >> >printed > >> >> >with %p. We have been getting demanding on this point lately in > >> >RDMA > >> >> >to enforce the ability to keep kernel pointers secret. > >> >> > > >> >> >> - wqe->sqe.sge[0].laddr = (u64)&wqe->sqe.sge[1]; > >> >> >> + wqe->sqe.sge[0].laddr = (uintptr_t)&wqe->sqe.sge[1]; > >> >> > > >> >> >[..] > >> >> > > >> >> >> rv = siw_rx_kva(srx, > >> >> >> - (void *)(sge->laddr + frx->sge_off), > >> >> >> + (void *)(uintptr_t)(sge->laddr + frx->sge_off), > >> >> >> sge_bytes); > >> >> > > >> >> >Bernard, this is nonsense, what is going on here with > >sge->laddr > >> >that > >> >> >it can't be a void *? > >> >> > > >> >> siw_sge is defined in siw-abi.h. We make the address u64 to keep > >> >the ABI > >> >> arch independent. > >> > > >> >Eh? How does the siw-abi.h store a kernel pointer? Sounds like > >kernel > >> >and user types are being mixed. > >> > > >> > >> siw-abi.h defines the work queue elements of a siw send queue. > >> For user land, the send queue is mmapped. Kernel or user land > >> clients write to its send queue when posting work > >> (SGE: buffer address, length, local key). > > > >Should have different types.. Don't want to accidently mix a laddr > >under user control with one under kernel control. > > > Well we have an unsigned 64bit for both user and kernel > application buffer addresses throughout the rdma stack, We do not. Kernel addresses are consistenyly void * or dma_addr_t Most places that consume a data address are using lkeys anyhow, which does have a lkey & u64, but that u64 is not a application buffer address, but the IOVA of the lkey, which is very different. I really have no idea why siw needs to mix kernel VAs with user pointers, particularly in wqes... Jason
-----"Jason Gunthorpe" <jgg@ziepe.ca> wrote: ----- >To: "Bernard Metzler" <BMT@zurich.ibm.com> >From: "Jason Gunthorpe" <jgg@ziepe.ca> >Date: 08/19/2019 05:07PM >Cc: "Geert Uytterhoeven" <geert@linux-m68k.org>, "Doug Ledford" ><dledford@redhat.com>, linux-rdma@vger.kernel.org, >linux-kernel@vger.kernel.org >Subject: [EXTERNAL] Re: Re: Re: [PATCH] RDMA/siw: Fix compiler >warnings on 32-bit due to u64/pointer abuse > >On Mon, Aug 19, 2019 at 02:52:13PM +0000, Bernard Metzler wrote: >> >> >To: "Bernard Metzler" <BMT@zurich.ibm.com> >> >From: "Jason Gunthorpe" <jgg@ziepe.ca> >> >Date: 08/19/2019 04:19PM >> >Cc: "Geert Uytterhoeven" <geert@linux-m68k.org>, "Doug Ledford" >> ><dledford@redhat.com>, linux-rdma@vger.kernel.org, >> >linux-kernel@vger.kernel.org >> >Subject: [EXTERNAL] Re: Re: Re: [PATCH] RDMA/siw: Fix compiler >> >warnings on 32-bit due to u64/pointer abuse >> > >> >On Mon, Aug 19, 2019 at 02:15:36PM +0000, Bernard Metzler wrote: >> >> >> >> >To: "Bernard Metzler" <BMT@zurich.ibm.com> >> >> >From: "Jason Gunthorpe" <jgg@ziepe.ca> >> >> >Date: 08/19/2019 03:52PM >> >> >Cc: "Geert Uytterhoeven" <geert@linux-m68k.org>, "Doug Ledford" >> >> ><dledford@redhat.com>, linux-rdma@vger.kernel.org, >> >> >linux-kernel@vger.kernel.org >> >> >Subject: [EXTERNAL] Re: Re: [PATCH] RDMA/siw: Fix compiler >> >warnings >> >> >on 32-bit due to u64/pointer abuse >> >> > >> >> >On Mon, Aug 19, 2019 at 01:36:11PM +0000, Bernard Metzler >wrote: >> >> >> >If the value is really a kernel pointer, then it ought to be >> >> >printed >> >> >> >with %p. We have been getting demanding on this point lately >in >> >> >RDMA >> >> >> >to enforce the ability to keep kernel pointers secret. >> >> >> > >> >> >> >> - wqe->sqe.sge[0].laddr = (u64)&wqe->sqe.sge[1]; >> >> >> >> + wqe->sqe.sge[0].laddr = (uintptr_t)&wqe->sqe.sge[1]; >> >> >> > >> >> >> >[..] >> >> >> > >> >> >> >> rv = siw_rx_kva(srx, >> >> >> >> - (void *)(sge->laddr + frx->sge_off), >> >> >> >> + (void *)(uintptr_t)(sge->laddr + frx->sge_off), >> >> >> >> sge_bytes); >> >> >> > >> >> >> >Bernard, this is nonsense, what is going on here with >> >sge->laddr >> >> >that >> >> >> >it can't be a void *? >> >> >> > >> >> >> siw_sge is defined in siw-abi.h. We make the address u64 to >keep >> >> >the ABI >> >> >> arch independent. >> >> > >> >> >Eh? How does the siw-abi.h store a kernel pointer? Sounds like >> >kernel >> >> >and user types are being mixed. >> >> > >> >> >> >> siw-abi.h defines the work queue elements of a siw send queue. >> >> For user land, the send queue is mmapped. Kernel or user land >> >> clients write to its send queue when posting work >> >> (SGE: buffer address, length, local key). >> > >> >Should have different types.. Don't want to accidently mix a laddr >> >under user control with one under kernel control. >> > >> Well we have an unsigned 64bit for both user and kernel >> application buffer addresses throughout the rdma stack, > >We do not. Kernel addresses are consistenyly void * or dma_addr_t > Absolutely. But these addresses are conveyed through the API as unsigned 64 during post_send(), and land in the siw send queue as is. During send queue processing, these addresses must be interpreted according to its context and transformed (casted) back to the callers intention. I frankly do not know what we can do differently... The representation of all addresses as unsigned 64 is given. Sorry for the confusion. >Most places that consume a data address are using lkeys anyhow, which >does have a lkey & u64, but that u64 is not a application buffer >address, but the IOVA of the lkey, which is very different. > >I really have no idea why siw needs to mix kernel VAs with user >pointers, particularly in wqes... > >Jason > >
On Mon, Aug 19, 2019 at 03:54:56PM +0000, Bernard Metzler wrote: > Absolutely. But these addresses are conveyed through the > API as unsigned 64 during post_send(), and land in the siw > send queue as is. During send queue processing, these addresses > must be interpreted according to its context and transformed > (casted) back to the callers intention. I frankly do not > know what we can do differently... The representation of > all addresses as unsigned 64 is given. Sorry for the confusion. send work does not have pointers in it, so I'm confused what this is about. Does siw allow userspace to stick an ordinary pointer for the SG list? The code paths here must be totally different, so there should be different types and functions for each case. Jason
-----"Jason Gunthorpe" <jgg@ziepe.ca> wrote: ----- >To: "Bernard Metzler" <BMT@zurich.ibm.com> >From: "Jason Gunthorpe" <jgg@ziepe.ca> >Date: 08/19/2019 06:05PM >Cc: "Geert Uytterhoeven" <geert@linux-m68k.org>, "Doug Ledford" ><dledford@redhat.com>, linux-rdma@vger.kernel.org, >linux-kernel@vger.kernel.org >Subject: [EXTERNAL] Re: Re: Re: Re: [PATCH] RDMA/siw: Fix compiler >warnings on 32-bit due to u64/pointer abuse > >On Mon, Aug 19, 2019 at 03:54:56PM +0000, Bernard Metzler wrote: > >> Absolutely. But these addresses are conveyed through the >> API as unsigned 64 during post_send(), and land in the siw >> send queue as is. During send queue processing, these addresses >> must be interpreted according to its context and transformed >> (casted) back to the callers intention. I frankly do not >> know what we can do differently... The representation of >> all addresses as unsigned 64 is given. Sorry for the confusion. > >send work does not have pointers in it, so I'm confused what this is >about. Does siw allow userspace to stick an ordinary pointer for the >SG list? Right a user references a buffer by address and local key it got during reservation of that buffer. The user can provide any VA between start of that buffer and registered length. > >The code paths here must be totally different, so there should be >different types and functions for each case. Yes, there is a function to process application memory (siw_rx_umem), to process a kernel PBL (siw_rx_pbl), and one to process kernel addresses (siw_rx_kva). Before running that function, the API representation of the current SGE gets translated into target buffer representation. Thanks and best regards, Bernard.
On Mon, Aug 19, 2019 at 04:29:11PM +0000, Bernard Metzler wrote: > > >To: "Bernard Metzler" <BMT@zurich.ibm.com> > >From: "Jason Gunthorpe" <jgg@ziepe.ca> > >Date: 08/19/2019 06:05PM > >Cc: "Geert Uytterhoeven" <geert@linux-m68k.org>, "Doug Ledford" > ><dledford@redhat.com>, linux-rdma@vger.kernel.org, > >linux-kernel@vger.kernel.org > >Subject: [EXTERNAL] Re: Re: Re: Re: [PATCH] RDMA/siw: Fix compiler > >warnings on 32-bit due to u64/pointer abuse > > > >On Mon, Aug 19, 2019 at 03:54:56PM +0000, Bernard Metzler wrote: > > > >> Absolutely. But these addresses are conveyed through the > >> API as unsigned 64 during post_send(), and land in the siw > >> send queue as is. During send queue processing, these addresses > >> must be interpreted according to its context and transformed > >> (casted) back to the callers intention. I frankly do not > >> know what we can do differently... The representation of > >> all addresses as unsigned 64 is given. Sorry for the confusion. > > > >send work does not have pointers in it, so I'm confused what this is > >about. Does siw allow userspace to stick an ordinary pointer for the > >SG list? > > Right a user references a buffer by address and local key it > got during reservation of that buffer. The user can provide any > VA between start of that buffer and registered length. Oh gross, it overloads the IOVA in the WR with a kernel void * ?? > >The code paths here must be totally different, so there should be > >different types and functions for each case. > > Yes, there is a function to process application memory (siw_rx_umem), > to process a kernel PBL (siw_rx_pbl), and one to process kernel > addresses (siw_rx_kva). Before running that function, the API > representation of the current SGE gets translated into target > buffer representation. Why does siw_pbl_get_buffer not return a void *?? Still looks like two types have been crammed together. The kernel can't ever store anything into the user WQE buffer, so I would think it would copy the buffer to kernel space, transform it properly and then refer to it as a kernel buffer. Kernel sourced buffers just skip the transofmration. JAson
On Mon, 2019-08-19 at 12:05 +0200, Geert Uytterhoeven wrote: > When compiling on 32-bit: > > drivers/infiniband/sw/siw/siw_cq.c:76:20: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > drivers/infiniband/sw/siw/siw_qp.c:952:28: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] [] > Fix this by applying the following rules: > 1. When printing a u64, the %llx format specififer should be used, > instead of casting to a pointer, and printing the latter. > 2. When assigning a pointer to a u64, the pointer should be cast to > uintptr_t, not u64, > 3. When casting from u64 to pointer, an intermediate cast to uintptr_t > should be added, I think a cast to unsigned long is rather more common. uintptr_t is used ~1300 times in the kernel. I believe a cast to unsigned long is much more common. It might be useful to add something to the Documentation for this style. Documentation/process/coding-style.rst And trivia: > > diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c [] > @@ -842,8 +842,8 @@ int siw_post_send(struct ib_qp *base_qp, const struct ib_send_wr *wr, > rv = -EINVAL; > break; > } > - siw_dbg_qp(qp, "opcode %d, flags 0x%x, wr_id 0x%p\n", > - sqe->opcode, sqe->flags, (void *)sqe->id); > + siw_dbg_qp(qp, "opcode %d, flags 0x%x, wr_id 0x%llx\n", > + sqe->opcode, sqe->flags, sqe->id); Printing possible pointers as %llx is generally not a good idea given the desire for %p obfuscation.
Hi Joe, On Mon, Aug 19, 2019 at 6:56 PM Joe Perches <joe@perches.com> wrote: > On Mon, 2019-08-19 at 12:05 +0200, Geert Uytterhoeven wrote: > > When compiling on 32-bit: > > > > drivers/infiniband/sw/siw/siw_cq.c:76:20: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > > drivers/infiniband/sw/siw/siw_qp.c:952:28: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] > [] > > Fix this by applying the following rules: > > 1. When printing a u64, the %llx format specififer should be used, > > instead of casting to a pointer, and printing the latter. > > 2. When assigning a pointer to a u64, the pointer should be cast to > > uintptr_t, not u64, > > 3. When casting from u64 to pointer, an intermediate cast to uintptr_t > > should be added, > > I think a cast to unsigned long is rather more common. > > uintptr_t is used ~1300 times in the kernel. > I believe a cast to unsigned long is much more common. That is true, as uintptr_t was introduced in C99. Similarly, unsigned long was used before size_t became common. However, nowadays size_t and uintptr_t are preferred. > It might be useful to add something to the Documentation > for this style. Documentation/process/coding-style.rst > > And trivia: > > > > diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c > [] > > @@ -842,8 +842,8 @@ int siw_post_send(struct ib_qp *base_qp, const struct ib_send_wr *wr, > > rv = -EINVAL; > > break; > > } > > - siw_dbg_qp(qp, "opcode %d, flags 0x%x, wr_id 0x%p\n", > > - sqe->opcode, sqe->flags, (void *)sqe->id); > > + siw_dbg_qp(qp, "opcode %d, flags 0x%x, wr_id 0x%llx\n", > > + sqe->opcode, sqe->flags, sqe->id); > > Printing possible pointers as %llx is generally not a good idea > given the desire for %p obfuscation. Are they pointers? Difficult to know with all the casting... Gr{oetje,eeting}s, Geert
--- Bernard Metzler, PhD Tech. Leader High Performance I/O, Principal Research Staff IBM Zurich Research Laboratory Saeumerstrasse 4 CH-8803 Rueschlikon, Switzerland +41 44 724 8605 -----"Geert Uytterhoeven" <geert@linux-m68k.org> wrote: ----- >To: "Joe Perches" <joe@perches.com> >From: "Geert Uytterhoeven" <geert@linux-m68k.org> >Date: 08/19/2019 07:15PM >Cc: "Bernard Metzler" <bmt@zurich.ibm.com>, "Doug Ledford" ><dledford@redhat.com>, "Jason Gunthorpe" <jgg@ziepe.ca>, "linux-rdma" ><linux-rdma@vger.kernel.org>, "Linux Kernel Mailing List" ><linux-kernel@vger.kernel.org> >Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Fix compiler warnings on >32-bit due to u64/pointer abuse > >Hi Joe, > >On Mon, Aug 19, 2019 at 6:56 PM Joe Perches <joe@perches.com> wrote: >> On Mon, 2019-08-19 at 12:05 +0200, Geert Uytterhoeven wrote: >> > When compiling on 32-bit: >> > >> > drivers/infiniband/sw/siw/siw_cq.c:76:20: warning: cast to >pointer from integer of different size [-Wint-to-pointer-cast] >> > drivers/infiniband/sw/siw/siw_qp.c:952:28: warning: cast from >pointer to integer of different size [-Wpointer-to-int-cast] >> [] >> > Fix this by applying the following rules: >> > 1. When printing a u64, the %llx format specififer should be >used, >> > instead of casting to a pointer, and printing the latter. >> > 2. When assigning a pointer to a u64, the pointer should be >cast to >> > uintptr_t, not u64, >> > 3. When casting from u64 to pointer, an intermediate cast to >uintptr_t >> > should be added, >> >> I think a cast to unsigned long is rather more common. >> >> uintptr_t is used ~1300 times in the kernel. >> I believe a cast to unsigned long is much more common. > >That is true, as uintptr_t was introduced in C99. >Similarly, unsigned long was used before size_t became common. > >However, nowadays size_t and uintptr_t are preferred. > >> It might be useful to add something to the Documentation >> for this style. Documentation/process/coding-style.rst >> >> And trivia: >> >> > > diff --git a/drivers/infiniband/sw/siw/siw_verbs.c >b/drivers/infiniband/sw/siw/siw_verbs.c >> [] >> > @@ -842,8 +842,8 @@ int siw_post_send(struct ib_qp *base_qp, >const struct ib_send_wr *wr, >> > rv = -EINVAL; >> > break; >> > } >> > - siw_dbg_qp(qp, "opcode %d, flags 0x%x, wr_id >0x%p\n", >> > - sqe->opcode, sqe->flags, (void >*)sqe->id); >> > + siw_dbg_qp(qp, "opcode %d, flags 0x%x, wr_id >0x%llx\n", >> > + sqe->opcode, sqe->flags, sqe->id); >> >> Printing possible pointers as %llx is generally not a good idea >> given the desire for %p obfuscation. > >Are they pointers? Difficult to know with all the casting... > You are right. This one is not a pointer. It is an application assigned unsigned 64bit. Just something (typically a pointer or array index) assigned by the application to match work completions with original work requests. So %llx would more correct here, and the cast is not needed then. Only problem that a kernel application typically puts a pointer into here (a pointer to a local context etc.). With the aim to support obfuscating the pointer for printout, we would be back to the cast plus %p formatting....? >Gr{oetje,eeting}s, > > Geert > >-- >Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- >geert@linux-m68k.org > >In personal conversations with technical people, I call myself a >hacker. But >when I'm talking to journalists I just say "programmer" or something >like that. > -- Linus Torvalds > >
-----"Jason Gunthorpe" <jgg@ziepe.ca> wrote: ----- >To: "Bernard Metzler" <BMT@zurich.ibm.com> >From: "Jason Gunthorpe" <jgg@ziepe.ca> >Date: 08/19/2019 06:35PM >Cc: "Geert Uytterhoeven" <geert@linux-m68k.org>, "Doug Ledford" ><dledford@redhat.com>, linux-rdma@vger.kernel.org, >linux-kernel@vger.kernel.org >Subject: [EXTERNAL] Re: Re: Re: Re: Re: [PATCH] RDMA/siw: Fix >compiler warnings on 32-bit due to u64/pointer abuse > >On Mon, Aug 19, 2019 at 04:29:11PM +0000, Bernard Metzler wrote: >> >> >To: "Bernard Metzler" <BMT@zurich.ibm.com> >> >From: "Jason Gunthorpe" <jgg@ziepe.ca> >> >Date: 08/19/2019 06:05PM >> >Cc: "Geert Uytterhoeven" <geert@linux-m68k.org>, "Doug Ledford" >> ><dledford@redhat.com>, linux-rdma@vger.kernel.org, >> >linux-kernel@vger.kernel.org >> >Subject: [EXTERNAL] Re: Re: Re: Re: [PATCH] RDMA/siw: Fix compiler >> >warnings on 32-bit due to u64/pointer abuse >> > >> >On Mon, Aug 19, 2019 at 03:54:56PM +0000, Bernard Metzler wrote: >> > >> >> Absolutely. But these addresses are conveyed through the >> >> API as unsigned 64 during post_send(), and land in the siw >> >> send queue as is. During send queue processing, these addresses >> >> must be interpreted according to its context and transformed >> >> (casted) back to the callers intention. I frankly do not >> >> know what we can do differently... The representation of >> >> all addresses as unsigned 64 is given. Sorry for the confusion. >> > >> >send work does not have pointers in it, so I'm confused what this >is >> >about. Does siw allow userspace to stick an ordinary pointer for >the >> >SG list? >> >> Right a user references a buffer by address and local key it >> got during reservation of that buffer. The user can provide any >> VA between start of that buffer and registered length. > >Oh gross, it overloads the IOVA in the WR with a kernel void * ?? Oh no. The user library writes the buffer address into the 64bit address field of the WR. This is nothing siw has invented. > >> >The code paths here must be totally different, so there should be >> >different types and functions for each case. >> >> Yes, there is a function to process application memory >(siw_rx_umem), >> to process a kernel PBL (siw_rx_pbl), and one to process kernel >> addresses (siw_rx_kva). Before running that function, the API >> representation of the current SGE gets translated into target >> buffer representation. > >Why does siw_pbl_get_buffer not return a void *?? > I think, in fact, it should be dma_addr_t, since this is what PBL's are described with. Makes sense? >Still looks like two types have been crammed together. > >The kernel can't ever store anything into the user WQE buffer, so I >would think it would copy the buffer to kernel space, transform it >properly and then refer to it as a kernel buffer. Kernel sourced >buffers just skip the transofmration. This is in fact what happens. siw just does not copy immediately during post send, but maintains a mmapped shared send queue. The kernel driver copies the current element from the send queue and processes it. Only then the current element gets transformed into the right buffer representation, since it is not being accessed before. Thanks Bernard. > >JAson > >
On Mon, Aug 19, 2019 at 05:39:04PM +0000, Bernard Metzler wrote: > > >To: "Bernard Metzler" <BMT@zurich.ibm.com> > >From: "Jason Gunthorpe" <jgg@ziepe.ca> > >Date: 08/19/2019 06:35PM > >Cc: "Geert Uytterhoeven" <geert@linux-m68k.org>, "Doug Ledford" > ><dledford@redhat.com>, linux-rdma@vger.kernel.org, > >linux-kernel@vger.kernel.org > >Subject: [EXTERNAL] Re: Re: Re: Re: Re: [PATCH] RDMA/siw: Fix > >compiler warnings on 32-bit due to u64/pointer abuse > > > >On Mon, Aug 19, 2019 at 04:29:11PM +0000, Bernard Metzler wrote: > >> > >> >To: "Bernard Metzler" <BMT@zurich.ibm.com> > >> >From: "Jason Gunthorpe" <jgg@ziepe.ca> > >> >Date: 08/19/2019 06:05PM > >> >Cc: "Geert Uytterhoeven" <geert@linux-m68k.org>, "Doug Ledford" > >> ><dledford@redhat.com>, linux-rdma@vger.kernel.org, > >> >linux-kernel@vger.kernel.org > >> >Subject: [EXTERNAL] Re: Re: Re: Re: [PATCH] RDMA/siw: Fix compiler > >> >warnings on 32-bit due to u64/pointer abuse > >> > > >> >On Mon, Aug 19, 2019 at 03:54:56PM +0000, Bernard Metzler wrote: > >> > > >> >> Absolutely. But these addresses are conveyed through the > >> >> API as unsigned 64 during post_send(), and land in the siw > >> >> send queue as is. During send queue processing, these addresses > >> >> must be interpreted according to its context and transformed > >> >> (casted) back to the callers intention. I frankly do not > >> >> know what we can do differently... The representation of > >> >> all addresses as unsigned 64 is given. Sorry for the confusion. > >> > > >> >send work does not have pointers in it, so I'm confused what this > >is > >> >about. Does siw allow userspace to stick an ordinary pointer for > >the > >> >SG list? > >> > >> Right a user references a buffer by address and local key it > >> got during reservation of that buffer. The user can provide any > >> VA between start of that buffer and registered length. > > > >Oh gross, it overloads the IOVA in the WR with a kernel void * ?? > > Oh no. The user library writes the buffer address into > the 64bit address field of the WR. This is nothing siw > has invented. No HW provider sticks pointers into the WR ring. It is either an iova & lkey pair, or SGE information is inlined into the WR ring. Never, ever, a user or kernel pointer. The closest we get to a kernel pointer is with the local dma lkey & iova == physical memory address. > >Why does siw_pbl_get_buffer not return a void *?? > > > I think, in fact, it should be dma_addr_t, since this is > what PBL's are described with. Makes sense? You mean because siw uses dma_virt_ops and can translate a dma_addr_t back to a pfn? Yes, that would make alot more sense. If all conversions went explicitly from a iova & lkey -> dma_addr_t -> void * in the kmap then I'd be a lot happier Jason
-----"Jason Gunthorpe" <jgg@ziepe.ca> wrote: ----- >To: "Bernard Metzler" <BMT@zurich.ibm.com> >From: "Jason Gunthorpe" <jgg@ziepe.ca> >Date: 08/19/2019 08:00PM >Cc: "Geert Uytterhoeven" <geert@linux-m68k.org>, "Doug Ledford" ><dledford@redhat.com>, linux-rdma@vger.kernel.org, >linux-kernel@vger.kernel.org >Subject: [EXTERNAL] Re: Re: Re: Re: Re: [PATCH] RDMA/siw: Fix >compiler warnings on 32-bit due to u64/pointer abuse > >On Mon, Aug 19, 2019 at 05:39:04PM +0000, Bernard Metzler wrote: >> >> >To: "Bernard Metzler" <BMT@zurich.ibm.com> >> >From: "Jason Gunthorpe" <jgg@ziepe.ca> >> >Date: 08/19/2019 06:35PM >> >Cc: "Geert Uytterhoeven" <geert@linux-m68k.org>, "Doug Ledford" >> ><dledford@redhat.com>, linux-rdma@vger.kernel.org, >> >linux-kernel@vger.kernel.org >> >Subject: [EXTERNAL] Re: Re: Re: Re: Re: [PATCH] RDMA/siw: Fix >> >compiler warnings on 32-bit due to u64/pointer abuse >> > >> >On Mon, Aug 19, 2019 at 04:29:11PM +0000, Bernard Metzler wrote: >> >> >> >> >To: "Bernard Metzler" <BMT@zurich.ibm.com> >> >> >From: "Jason Gunthorpe" <jgg@ziepe.ca> >> >> >Date: 08/19/2019 06:05PM >> >> >Cc: "Geert Uytterhoeven" <geert@linux-m68k.org>, "Doug Ledford" >> >> ><dledford@redhat.com>, linux-rdma@vger.kernel.org, >> >> >linux-kernel@vger.kernel.org >> >> >Subject: [EXTERNAL] Re: Re: Re: Re: [PATCH] RDMA/siw: Fix >compiler >> >> >warnings on 32-bit due to u64/pointer abuse >> >> > >> >> >On Mon, Aug 19, 2019 at 03:54:56PM +0000, Bernard Metzler >wrote: >> >> > >> >> >> Absolutely. But these addresses are conveyed through the >> >> >> API as unsigned 64 during post_send(), and land in the siw >> >> >> send queue as is. During send queue processing, these >addresses >> >> >> must be interpreted according to its context and transformed >> >> >> (casted) back to the callers intention. I frankly do not >> >> >> know what we can do differently... The representation of >> >> >> all addresses as unsigned 64 is given. Sorry for the >confusion. >> >> > >> >> >send work does not have pointers in it, so I'm confused what >this >> >is >> >> >about. Does siw allow userspace to stick an ordinary pointer >for >> >the >> >> >SG list? >> >> >> >> Right a user references a buffer by address and local key it >> >> got during reservation of that buffer. The user can provide any >> >> VA between start of that buffer and registered length. >> > >> >Oh gross, it overloads the IOVA in the WR with a kernel void * ?? >> >> Oh no. The user library writes the buffer address into >> the 64bit address field of the WR. This is nothing siw >> has invented. > >No HW provider sticks pointers into the WR ring. Now siw is a SW only provider. It sits on top of TCP kernel sockets. siw translates any local application buffer reference it gets back into a kvec or page pointer (transmit from), or a virtual address (receive into). This is what the TCP interface wants. In fact, siw cares about physical addresses only since the RDMA (kernel level) user may care about it. It translates those back into something the TCP interface can consume. > >It is either an iova & lkey pair, or SGE information is inlined into >the WR ring. > In siw, the reference to any type of memory is kept uninterpreted in the send/receive queue until it gets accessed by a data transfer. The information on what type of memory is being referenced is deducted from the local memory key. As said, this step is being executed only when the actual buffer is to be touched. All it needs before that translation is to keep the 32bit key + length and the up to 64bit address in a work queue element within the send queue. lkey lookup and memory translation + access validation happens after the work queue element left the send/receive queue and a local copy of it is being processed by the kernel driver during RX or TX operations. Inline data is implemented similar to how HW providers do it - user data are copied immediately into the WR array. >Never, ever, a user or kernel pointer. > >The closest we get to a kernel pointer is with the local dma lkey & >iova == physical memory address. > >> >Why does siw_pbl_get_buffer not return a void *?? >> >> >> I think, in fact, it should be dma_addr_t, since this is >> what PBL's are described with. Makes sense? > >You mean because siw uses dma_virt_ops and can translate a dma_addr_t >back to a pfn? Yes, that would make alot more sense. > >If all conversions went explicitly from a iova & lkey -> dma_addr_t >-> void * in >the kmap then I'd be a lot happier > >Jason > >
On Mon, Aug 19, 2019 at 09:40:10PM +0000, Bernard Metzler wrote: > >It is either an iova & lkey pair, or SGE information is inlined into > >the WR ring. > > > In siw, the reference to any type of memory is kept uninterpreted > in the send/receive queue until it gets accessed by a data > transfer. The information on what type of memory is being referenced > is deducted from the local memory key. As said, this step is > being executed only when the actual buffer is to be touched. > All it needs before that translation is to keep the 32bit key + > length and the up to 64bit address in a work queue element within > the send queue. > lkey lookup and memory translation + access validation happens > after the work queue element left the send/receive queue and a > local copy of it is being processed by the kernel driver > during RX or TX operations. > > Inline data is implemented similar to how HW providers do > it - user data are copied immediately into the WR array. I still don't understand how kernel void *'s are getting into WQEs. Jason
From: Geert Uytterhoeven > Sent: 19 August 2019 18:15 ... > > I think a cast to unsigned long is rather more common. > > > > uintptr_t is used ~1300 times in the kernel. > > I believe a cast to unsigned long is much more common. > > That is true, as uintptr_t was introduced in C99. > Similarly, unsigned long was used before size_t became common. > > However, nowadays size_t and uintptr_t are preferred. Isn't uintptr_t defined by the same standard as uint32_t? If the former is allowed so should the latter. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hi David, On Tue, Aug 27, 2019 at 4:17 PM David Laight <David.Laight@aculab.com> wrote: > From: Geert Uytterhoeven > > Sent: 19 August 2019 18:15 > ... > > > I think a cast to unsigned long is rather more common. > > > > > > uintptr_t is used ~1300 times in the kernel. > > > I believe a cast to unsigned long is much more common. > > > > That is true, as uintptr_t was introduced in C99. > > Similarly, unsigned long was used before size_t became common. > > > > However, nowadays size_t and uintptr_t are preferred. > > Isn't uintptr_t defined by the same standard as uint32_t? I believe so. > If the former is allowed so should the latter. You mean the other way around? Gr{oetje,eeting}s, Geert
On Tue, Aug 27, 2019 at 07:29:52PM +0200, Geert Uytterhoeven wrote: > Hi David, > > On Tue, Aug 27, 2019 at 4:17 PM David Laight <David.Laight@aculab.com> wrote: > > From: Geert Uytterhoeven > > > Sent: 19 August 2019 18:15 > > ... > > > > I think a cast to unsigned long is rather more common. > > > > > > > > uintptr_t is used ~1300 times in the kernel. > > > > I believe a cast to unsigned long is much more common. > > > > > > That is true, as uintptr_t was introduced in C99. > > > Similarly, unsigned long was used before size_t became common. > > > > > > However, nowadays size_t and uintptr_t are preferred. > > > > Isn't uintptr_t defined by the same standard as uint32_t? > > I believe so. It sure as hell is not. C99 7.18.1.4: The following type designates an unsigned integer type with the property that any valid pointer to void can be converted to this type, then converted back to pointer to void, and the result will compare equal to the original pointer: uintptr_t IOW, it's "large enough to represent pointers".
Hi Al, On Tue, Aug 27, 2019 at 7:46 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > On Tue, Aug 27, 2019 at 07:29:52PM +0200, Geert Uytterhoeven wrote: > > On Tue, Aug 27, 2019 at 4:17 PM David Laight <David.Laight@aculab.com> wrote: > > > From: Geert Uytterhoeven > > > > Sent: 19 August 2019 18:15 > > > ... > > > > > I think a cast to unsigned long is rather more common. > > > > > > > > > > uintptr_t is used ~1300 times in the kernel. > > > > > I believe a cast to unsigned long is much more common. > > > > > > > > That is true, as uintptr_t was introduced in C99. > > > > Similarly, unsigned long was used before size_t became common. > > > > > > > > However, nowadays size_t and uintptr_t are preferred. > > > > > > Isn't uintptr_t defined by the same standard as uint32_t? > > > > I believe so. > > It sure as hell is not. C99 7.18.1.4: > > The following type designates an unsigned integer type with the property that any valid > pointer to void can be converted to this type, then converted back to pointer to void, > and the result will compare equal to the original pointer: > uintptr_t > > IOW, it's "large enough to represent pointers". I did not say the two types are identical, and can be used interchangeable. Both types are defined (at least) in https://pubs.opengroup.org/onlinepubs/009695399/basedefs/stdint.h.html Gr{oetje,eeting}s, Geert
On Tue, 2019-08-27 at 19:59 +0200, Geert Uytterhoeven wrote: > On Tue, Aug 27, 2019 at 7:46 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Tue, Aug 27, 2019 at 07:29:52PM +0200, Geert Uytterhoeven wrote: > > > On Tue, Aug 27, 2019 at 4:17 PM David Laight <David.Laight@aculab.com> wrote: > > > > From: Geert Uytterhoeven > > > > > Sent: 19 August 2019 18:15 > > > > ... > > > > > > I think a cast to unsigned long is rather more common. > > > > > > > > > > > > uintptr_t is used ~1300 times in the kernel. > > > > > > I believe a cast to unsigned long is much more common. btw: apparently that's not true. This grep may be incomplete but it seems there are fewer kernel uses of a cast to unsigned long then pointer: $ git grep -P '\(\s*\w+(\s+\w+){0,3}(\s*\*)+\s*\)\s*\(\s*unsigned\s+long\s*\)'|wc -l 423 Maybe add a cast_to_ptr macro like #define cast_to_ptr(type, val) ((type)(uintptr_t)(val)) though that may not save any horizontal space and/or a checkpatch test like: --- scripts/checkpatch.pl | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 287fe73688f0..4ec88bc53f2f 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -6281,6 +6281,15 @@ sub process { } } +# check for casts to pointer with intermediate casts to (unsigned long) not (uintptr_t) + if ($line =~ /\(\s*\w+(?:\s+\w+){0,3}(?:\s*\*){1,4}\s*\)\s*\(\s*unsigned\s+long\s*\)/) { + if (WARN("PREFER_UINTPTR_T", + "prefer intermediate cast to uintptr_t\n" . $herecurr) && + $fix) { + $fixed[$fixlinenr] =~ s/(\(\s*\w+(?:\s+\w+){0,3}(?:\s*\*){1,4}\s*\))\s*\(\s*unsigned\s+long\s*\)/$1(uintptr_t)/; + } + } + # check for pointless casting of alloc functions if ($line =~ /\*\s*\)\s*$allocFunctions\b/) { WARN("UNNECESSARY_CASTS",
From: Joe Perches > Sent: 27 August 2019 19:33 > On Tue, 2019-08-27 at 19:59 +0200, Geert Uytterhoeven wrote: > > On Tue, Aug 27, 2019 at 7:46 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > > On Tue, Aug 27, 2019 at 07:29:52PM +0200, Geert Uytterhoeven wrote: > > > > On Tue, Aug 27, 2019 at 4:17 PM David Laight <David.Laight@aculab.com> wrote: > > > > > From: Geert Uytterhoeven > > > > > > Sent: 19 August 2019 18:15 > > > > > ... > > > > > > > I think a cast to unsigned long is rather more common. > > > > > > > > > > > > > > uintptr_t is used ~1300 times in the kernel. > > > > > > > I believe a cast to unsigned long is much more common. > > btw: apparently that's not true. > > This grep may be incomplete but it seems there are fewer > kernel uses of a cast to unsigned long then pointer: > > $ git grep -P '\(\s*\w+(\s+\w+){0,3}(\s*\*)+\s*\)\s*\(\s*unsigned\s+long\s*\)'|wc -l > 423 > > Maybe add a cast_to_ptr macro like > > #define cast_to_ptr(type, val) ((type)(uintptr_t)(val)) > > though that may not save any horizontal space And it is another bit of pointless obfuscation.... David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/drivers/infiniband/sw/siw/siw_cq.c b/drivers/infiniband/sw/siw/siw_cq.c index e381ae9b7d62498e..f4ec26eeb9df62bf 100644 --- a/drivers/infiniband/sw/siw/siw_cq.c +++ b/drivers/infiniband/sw/siw/siw_cq.c @@ -71,9 +71,10 @@ int siw_reap_cqe(struct siw_cq *cq, struct ib_wc *wc) wc->wc_flags = IB_WC_WITH_INVALIDATE; } wc->qp = cqe->base_qp; - siw_dbg_cq(cq, "idx %u, type %d, flags %2x, id 0x%p\n", + siw_dbg_cq(cq, + "idx %u, type %d, flags %2x, id 0x%llx\n", cq->cq_get % cq->num_cqe, cqe->opcode, - cqe->flags, (void *)cqe->id); + cqe->flags, cqe->id); } WRITE_ONCE(cqe->flags, 0); cq->cq_get++; diff --git a/drivers/infiniband/sw/siw/siw_qp.c b/drivers/infiniband/sw/siw/siw_qp.c index 0990307c5d2cde95..430314c8abd948c2 100644 --- a/drivers/infiniband/sw/siw/siw_qp.c +++ b/drivers/infiniband/sw/siw/siw_qp.c @@ -949,7 +949,7 @@ int siw_activate_tx(struct siw_qp *qp) rv = -EINVAL; goto out; } - wqe->sqe.sge[0].laddr = (u64)&wqe->sqe.sge[1]; + wqe->sqe.sge[0].laddr = (uintptr_t)&wqe->sqe.sge[1]; wqe->sqe.sge[0].lkey = 0; wqe->sqe.num_sge = 1; } diff --git a/drivers/infiniband/sw/siw/siw_qp_rx.c b/drivers/infiniband/sw/siw/siw_qp_rx.c index f87657a11657223e..8c183fc00ffd9846 100644 --- a/drivers/infiniband/sw/siw/siw_qp_rx.c +++ b/drivers/infiniband/sw/siw/siw_qp_rx.c @@ -38,9 +38,9 @@ static int siw_rx_umem(struct siw_rx_stream *srx, struct siw_umem *umem, p = siw_get_upage(umem, dest_addr); if (unlikely(!p)) { - pr_warn("siw: %s: [QP %u]: bogus addr: %p, %p\n", + pr_warn("siw: %s: [QP %u]: bogus addr: %llx, %llx\n", __func__, qp_id(rx_qp(srx)), - (void *)dest_addr, (void *)umem->fp_addr); + dest_addr, umem->fp_addr); /* siw internal error */ srx->skb_copied += copied; srx->skb_new -= copied; @@ -138,7 +138,8 @@ static int siw_rx_pbl(struct siw_rx_stream *srx, int *pbl_idx, break; bytes = min(bytes, len); - if (siw_rx_kva(srx, (void *)buf_addr, bytes) == bytes) { + if (siw_rx_kva(srx, (void *)(uintptr_t)buf_addr, bytes) + == bytes) { copied += bytes; offset += bytes; len -= bytes; @@ -485,7 +486,7 @@ int siw_proc_send(struct siw_qp *qp) mem_p = *mem; if (mem_p->mem_obj == NULL) rv = siw_rx_kva(srx, - (void *)(sge->laddr + frx->sge_off), + (void *)(uintptr_t)(sge->laddr + frx->sge_off), sge_bytes); else if (!mem_p->is_pbl) rv = siw_rx_umem(srx, mem_p->umem, @@ -598,7 +599,7 @@ int siw_proc_write(struct siw_qp *qp) if (mem->mem_obj == NULL) rv = siw_rx_kva(srx, - (void *)(srx->ddp_to + srx->fpdu_part_rcvd), + (void *)(uintptr_t)(srx->ddp_to + srx->fpdu_part_rcvd), bytes); else if (!mem->is_pbl) rv = siw_rx_umem(srx, mem->umem, @@ -841,8 +842,9 @@ int siw_proc_rresp(struct siw_qp *qp) bytes = min(srx->fpdu_part_rem, srx->skb_new); if (mem_p->mem_obj == NULL) - rv = siw_rx_kva(srx, (void *)(sge->laddr + wqe->processed), - bytes); + rv = siw_rx_kva(srx, + (void *)(uintptr_t)(sge->laddr + wqe->processed), + bytes); else if (!mem_p->is_pbl) rv = siw_rx_umem(srx, mem_p->umem, sge->laddr + wqe->processed, bytes); diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c b/drivers/infiniband/sw/siw/siw_qp_tx.c index 43020d2040fc3394..311d682539b5232b 100644 --- a/drivers/infiniband/sw/siw/siw_qp_tx.c +++ b/drivers/infiniband/sw/siw/siw_qp_tx.c @@ -41,6 +41,7 @@ static int siw_try_1seg(struct siw_iwarp_tx *c_tx, u64 paddr) { struct siw_wqe *wqe = &c_tx->wqe_active; struct siw_sge *sge = &wqe->sqe.sge[0]; + void *dst = (void *)(uintptr_t)paddr; u32 bytes = sge->length; if (bytes > MAX_HDR_INLINE || wqe->sqe.num_sge != 1) @@ -50,16 +51,16 @@ static int siw_try_1seg(struct siw_iwarp_tx *c_tx, u64 paddr) return 0; if (tx_flags(wqe) & SIW_WQE_INLINE) { - memcpy((void *)paddr, &wqe->sqe.sge[1], bytes); + memcpy(dst, &wqe->sqe.sge[1], bytes); } else { struct siw_mem *mem = wqe->mem[0]; if (!mem->mem_obj) { /* Kernel client using kva */ - memcpy((void *)paddr, (void *)sge->laddr, bytes); + memcpy(dst, (void *)(uintptr_t)sge->laddr, bytes); } else if (c_tx->in_syscall) { - if (copy_from_user((void *)paddr, - (const void __user *)sge->laddr, + if (copy_from_user(dst, + (const void __user *)(uintptr_t)sge->laddr, bytes)) return -EFAULT; } else { @@ -79,12 +80,12 @@ static int siw_try_1seg(struct siw_iwarp_tx *c_tx, u64 paddr) buffer = kmap_atomic(p); if (likely(PAGE_SIZE - off >= bytes)) { - memcpy((void *)paddr, buffer + off, bytes); + memcpy(dst, buffer + off, bytes); kunmap_atomic(buffer); } else { unsigned long part = bytes - (PAGE_SIZE - off); - memcpy((void *)paddr, buffer + off, part); + memcpy(dst, buffer + off, part); kunmap_atomic(buffer); if (!mem->is_pbl) @@ -98,8 +99,7 @@ static int siw_try_1seg(struct siw_iwarp_tx *c_tx, u64 paddr) return -EFAULT; buffer = kmap_atomic(p); - memcpy((void *)(paddr + part), buffer, - bytes - part); + memcpy(dst + part, buffer, bytes - part); kunmap_atomic(buffer); } } @@ -166,7 +166,7 @@ static int siw_qp_prepare_tx(struct siw_iwarp_tx *c_tx) c_tx->ctrl_len = sizeof(struct iwarp_send); crc = (char *)&c_tx->pkt.send_pkt.crc; - data = siw_try_1seg(c_tx, (u64)crc); + data = siw_try_1seg(c_tx, (uintptr_t)crc); break; case SIW_OP_SEND_REMOTE_INV: @@ -189,7 +189,7 @@ static int siw_qp_prepare_tx(struct siw_iwarp_tx *c_tx) c_tx->ctrl_len = sizeof(struct iwarp_send_inv); crc = (char *)&c_tx->pkt.send_pkt.crc; - data = siw_try_1seg(c_tx, (u64)crc); + data = siw_try_1seg(c_tx, (uintptr_t)crc); break; case SIW_OP_WRITE: @@ -201,7 +201,7 @@ static int siw_qp_prepare_tx(struct siw_iwarp_tx *c_tx) c_tx->ctrl_len = sizeof(struct iwarp_rdma_write); crc = (char *)&c_tx->pkt.write_pkt.crc; - data = siw_try_1seg(c_tx, (u64)crc); + data = siw_try_1seg(c_tx, (uintptr_t)crc); break; case SIW_OP_READ_RESPONSE: @@ -216,7 +216,7 @@ static int siw_qp_prepare_tx(struct siw_iwarp_tx *c_tx) c_tx->ctrl_len = sizeof(struct iwarp_rdma_rresp); crc = (char *)&c_tx->pkt.write_pkt.crc; - data = siw_try_1seg(c_tx, (u64)crc); + data = siw_try_1seg(c_tx, (uintptr_t)crc); break; default: @@ -473,7 +473,8 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s) * tx from kernel virtual address: either inline data * or memory region with assigned kernel buffer */ - iov[seg].iov_base = (void *)(sge->laddr + sge_off); + iov[seg].iov_base = + (void *)(uintptr_t)(sge->laddr + sge_off); iov[seg].iov_len = sge_len; if (do_crc) @@ -532,7 +533,7 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s) if (do_crc) crypto_shash_update( c_tx->mpa_crc_hd, - (void *)(sge->laddr + sge_off), + (void *)(uintptr_t)(sge->laddr + sge_off), plen); } @@ -829,7 +830,8 @@ static int siw_qp_sq_proc_tx(struct siw_qp *qp, struct siw_wqe *wqe) rv = -EINVAL; goto tx_error; } - wqe->sqe.sge[0].laddr = (u64)&wqe->sqe.sge[1]; + wqe->sqe.sge[0].laddr = + (uintptr_t)&wqe->sqe.sge[1]; } } wqe->wr_status = SIW_WR_INPROGRESS; @@ -924,7 +926,7 @@ static int siw_qp_sq_proc_tx(struct siw_qp *qp, struct siw_wqe *wqe) static int siw_fastreg_mr(struct ib_pd *pd, struct siw_sqe *sqe) { - struct ib_mr *base_mr = (struct ib_mr *)sqe->base_mr; + struct ib_mr *base_mr = (struct ib_mr *)(uintptr_t)sqe->base_mr; struct siw_device *sdev = to_siw_dev(pd->device); struct siw_mem *mem = siw_mem_id2obj(sdev, sqe->rkey >> 8); int rv = 0; diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c index e7f3a2379d9d8785..0672ff39a5bcf02d 100644 --- a/drivers/infiniband/sw/siw/siw_verbs.c +++ b/drivers/infiniband/sw/siw/siw_verbs.c @@ -662,7 +662,7 @@ static int siw_copy_inline_sgl(const struct ib_send_wr *core_wr, void *kbuf = &sqe->sge[1]; int num_sge = core_wr->num_sge, bytes = 0; - sqe->sge[0].laddr = (u64)kbuf; + sqe->sge[0].laddr = (uintptr_t)kbuf; sqe->sge[0].lkey = 0; while (num_sge--) { @@ -825,7 +825,7 @@ int siw_post_send(struct ib_qp *base_qp, const struct ib_send_wr *wr, break; case IB_WR_REG_MR: - sqe->base_mr = (uint64_t)reg_wr(wr)->mr; + sqe->base_mr = (uintptr_t)reg_wr(wr)->mr; sqe->rkey = reg_wr(wr)->key; sqe->access = reg_wr(wr)->access & IWARP_ACCESS_MASK; sqe->opcode = SIW_OP_REG_MR; @@ -842,8 +842,8 @@ int siw_post_send(struct ib_qp *base_qp, const struct ib_send_wr *wr, rv = -EINVAL; break; } - siw_dbg_qp(qp, "opcode %d, flags 0x%x, wr_id 0x%p\n", - sqe->opcode, sqe->flags, (void *)sqe->id); + siw_dbg_qp(qp, "opcode %d, flags 0x%x, wr_id 0x%llx\n", + sqe->opcode, sqe->flags, sqe->id); if (unlikely(rv < 0)) break;
When compiling on 32-bit: drivers/infiniband/sw/siw/siw_cq.c:76:20: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] drivers/infiniband/sw/siw/siw_qp.c:952:28: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] drivers/infiniband/sw/siw/siw_qp_tx.c:53:10: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] drivers/infiniband/sw/siw/siw_qp_tx.c:59:11: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] drivers/infiniband/sw/siw/siw_qp_tx.c:59:26: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] drivers/infiniband/sw/siw/siw_qp_tx.c:61:23: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] drivers/infiniband/sw/siw/siw_qp_tx.c:62:9: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] drivers/infiniband/sw/siw/siw_qp_tx.c:82:12: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] drivers/infiniband/sw/siw/siw_qp_tx.c:87:12: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] drivers/infiniband/sw/siw/siw_qp_tx.c:101:12: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] drivers/infiniband/sw/siw/siw_qp_tx.c:169:29: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] drivers/infiniband/sw/siw/siw_qp_tx.c:192:29: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] drivers/infiniband/sw/siw/siw_qp_tx.c:204:29: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] drivers/infiniband/sw/siw/siw_qp_tx.c:219:29: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] drivers/infiniband/sw/siw/siw_qp_tx.c:476:24: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] drivers/infiniband/sw/siw/siw_qp_tx.c:535:7: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] drivers/infiniband/sw/siw/siw_qp_tx.c:832:29: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] drivers/infiniband/sw/siw/siw_qp_tx.c:927:26: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] drivers/infiniband/sw/siw/siw_qp_rx.c:43:5: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] drivers/infiniband/sw/siw/siw_qp_rx.c:43:24: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] drivers/infiniband/sw/siw/siw_qp_rx.c:141:23: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] drivers/infiniband/sw/siw/siw_qp_rx.c:488:6: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] drivers/infiniband/sw/siw/siw_qp_rx.c:601:5: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] drivers/infiniband/sw/siw/siw_qp_rx.c:844:24: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] drivers/infiniband/sw/siw/siw_verbs.c:665:22: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] drivers/infiniband/sw/siw/siw_verbs.c:828:19: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] drivers/infiniband/sw/siw/siw_verbs.c:846:32: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] Fix this by applying the following rules: 1. When printing a u64, the %llx format specififer should be used, instead of casting to a pointer, and printing the latter. 2. When assigning a pointer to a u64, the pointer should be cast to uintptr_t, not u64, 3. When casting from u64 to pointer, an intermediate cast to uintptr_t should be added, Fixes: 2c8ccb37b08fe364 ("RDMA/siw: Change CQ flags from 64->32 bits") Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> --- The issues predate the commit mentioned above, but didn't become visible before. The Right Thing(TM) would be to get rid of all this casting, and use proper types instead. This would involve teaching the siw people that a kernel virtual address is not called a physical address, and should not use u64. --- drivers/infiniband/sw/siw/siw_cq.c | 5 ++-- drivers/infiniband/sw/siw/siw_qp.c | 2 +- drivers/infiniband/sw/siw/siw_qp_rx.c | 16 +++++++------ drivers/infiniband/sw/siw/siw_qp_tx.c | 34 ++++++++++++++------------- drivers/infiniband/sw/siw/siw_verbs.c | 8 +++---- 5 files changed, 35 insertions(+), 30 deletions(-)