| Message ID | 20210324141337.29269-1-ciara.loftus@intel.com |
|---|---|
| Headers | show |
| Series | AF_XDP Socket Creation Fixes | expand |
On Wed, Mar 24, 2021 at 3:46 PM Ciara Loftus <ciara.loftus@intel.com> wrote: > > During xsk_socket__create the XDP_RX_RING and XDP_TX_RING setsockopts > are called to create the rx and tx rings for the AF_XDP socket. If the ring > has already been set up, the setsockopt will return an error. However, > in the event of a failure during xsk_socket__create(_shared) after the > rings have been set up, the user may wish to retry the socket creation > using these pre-existing rings. In this case we can ignore the error > returned by the setsockopts. If there is a true error, the subsequent > call to mmap() will catch it. > > Fixes: 1cad07884239 ("libbpf: add support for using AF_XDP sockets") > > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> > --- > tools/lib/bpf/xsk.c | 34 ++++++++++++++++------------------ > 1 file changed, 16 insertions(+), 18 deletions(-) > > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > index ec3c23299329..1f1c4c11c292 100644 > --- a/tools/lib/bpf/xsk.c > +++ b/tools/lib/bpf/xsk.c > @@ -904,24 +904,22 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr, > } > xsk->ctx = ctx; > > - if (rx) { > - err = setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING, > - &xsk->config.rx_size, > - sizeof(xsk->config.rx_size)); > - if (err) { > - err = -errno; > - goto out_put_ctx; > - } > - } > - if (tx) { > - err = setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING, > - &xsk->config.tx_size, > - sizeof(xsk->config.tx_size)); > - if (err) { > - err = -errno; > - goto out_put_ctx; > - } > - } > + /* The return values of these setsockopt calls are intentionally not checked. > + * If the ring has already been set up setsockopt will return an error. However, > + * this scenario is acceptable as the user may be retrying the socket creation > + * with rings which were set up in a previous but ultimately unsuccessful call > + * to xsk_socket__create(_shared). The call later to mmap() will fail if there > + * is a real issue and we handle that return value appropriately there. > + */ > + if (rx) > + setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING, > + &xsk->config.rx_size, > + sizeof(xsk->config.rx_size)); > + > + if (tx) > + setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING, > + &xsk->config.tx_size, > + sizeof(xsk->config.tx_size)); Thanks Ciara! This is a pragmatic solution, but I do not see any better way around it since these operations are irreversible. And it works without any fix to the kernel which is good and you have a comment explaining things clearly. With that said, it would be nice as a follow up to bpf-next to actually return a unique error value (among the ones that this function can return) when the rings have already been mapped. This way the user can react to this in a more informed way in the future. Acked-by: Magnus Karlsson <magnus.karlsson@intel.com> > > err = xsk_get_mmap_offsets(xsk->fd, &off); > if (err) { > -- > 2.17.1 >
On Wed, Mar 24, 2021 at 3:46 PM Ciara Loftus <ciara.loftus@intel.com> wrote: > > Calls to xsk_socket__create dereference the umem to access the > fill_save and comp_save pointers. Make sure the umem is non-NULL > before doing this. > > Fixes: 2f6324a3937f ("libbpf: Support shared umems between queues and devices") > > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> > --- > tools/lib/bpf/xsk.c | 3 +++ > 1 file changed, 3 insertions(+) Thank you for the fix! Acked-by: Magnus Karlsson <magnus.karlsson@intel.com> > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > index 526fc35c0b23..443b0cfb45e8 100644 > --- a/tools/lib/bpf/xsk.c > +++ b/tools/lib/bpf/xsk.c > @@ -1019,6 +1019,9 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname, > struct xsk_ring_cons *rx, struct xsk_ring_prod *tx, > const struct xsk_socket_config *usr_config) > { > + if (!umem) > + return -EFAULT; > + > return xsk_socket__create_shared(xsk_ptr, ifname, queue_id, umem, > rx, tx, umem->fill_save, > umem->comp_save, usr_config); > -- > 2.17.1 >