Re: [PATCH for-next 6/6] RDMA/rxe: Fix redundant skb_put_zero

[Zhu Yanjun <zyjzyj2000@gmail.com>]

On Fri, Jun 18, 2021 at 11:32 PM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>
> On 6/18/21 3:02 AM, Zhu Yanjun wrote:
> > On Fri, Jun 18, 2021 at 1:00 PM Bob Pearson <rpearsonhpe@gmail.com> wrote:
> >>
> >> rxe_init_packet() in rxe_net.c calls skb_put_zero() to reserve space
> >> for the payload and zero it out. All these bytes are then re-written
> >> with RoCE headers and payload. Remove this useless extra copy.
> >
> > The paylen seems to be a variable, that is, the length of pkt->hdr is not fixed.
> >
> > Can you confirm that all the pkt->hdr are re-writtenwith RoCE headers
> > and payload?
>
> Yes. rxe_init_packet() is called twice, once from rxe_req.c for request packets and once from rxe_resp.c for response packets.
> In rxe_req.c in init_req_packet() paylen is set to
>
>     paylen = rxe_opcode[opcode].length + payload + pad + RXE_ICRC_SIZE
>
> which is the correct size of the packet from the UDP header to the frame FCS i.e. the UDP payload. rxe_opcode[opcode] is a table that includes the length of the all the RoCE headers for a given opcode which does vary. Payload is the RoCE payload and pad is the number of pad bytes required to extend the payload to a multiple of 4 bytes. RXE_ICRC_SIZE is the 4 bytes for the RoCE invariant CRC. It requires some checking but all the headers are fully written, the payload is fully copied from the client and the pad and ICRC bytes are also written. In rxe_resp.c paylen is set to the same value.

Too complicated assignment.
So I prefer to skb_put_zero.

Zhu Yanjun
>
> There are two potential issues here 1) Is the intended packet sent to the destination, and 2) is there a possibility that information can leak from the kernel to the outside. The above addresses 1). 2) requires the assumption that the NIC is not examining data outside of the proper data area in the skb and doing something with it. But you have a worse problem there since the NIC has DMA access to all of kernel memory and can send any packet it likes.
>
> Bob Pearson
>
> > Zhu Yanjun
> >
> >>
> >> Fixes: ecb238f6a7f3 ("IB/cxgb4: use skb_put_zero()/__skb_put_zero")
> >> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
> >> ---
> >>  drivers/infiniband/sw/rxe/rxe_net.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
> >> index 178a66a45312..6605ee777667 100644
> >> --- a/drivers/infiniband/sw/rxe/rxe_net.c
> >> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> >> @@ -470,7 +470,7 @@ struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
> >>
> >>         pkt->rxe        = rxe;
> >>         pkt->port_num   = port_num;
> >> -       pkt->hdr        = skb_put_zero(skb, paylen);
> >> +       pkt->hdr        = skb_put(skb, paylen);
> >>         pkt->mask       |= RXE_GRH_MASK;
> >>
> >>  out:
> >> --
> >> 2.30.2
> >>
>