| Message ID | 20210324141337.29269-4-ciara.loftus@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| 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 >
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)); err = xsk_get_mmap_offsets(xsk->fd, &off); if (err) {
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(-)