On Mon, 6 May 2019 at 10:26, Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
>
> On 04/30/2019 02:45 PM, Björn Töpel wrote:
> > From: Björn Töpel <bjorn.topel@xxxxxxxxx>
> >
> > When unmapping the AF_XDP memory regions used for the rings, an
> > invalid address was passed to the munmap() calls. Instead of passing
> > the beginning of the memory region, the descriptor region was passed
> > to munmap.
> >
> > When the userspace application tried to tear down an AF_XDP socket,
> > the operation failed and the application would still have a reference
> > to socket it wished to get rid of.
> >
> > Reported-by: William Tu <u9012063@xxxxxxxxx>
> > Fixes: 1cad07884239 ("libbpf: add support for using AF_XDP sockets")
> > Signed-off-by: Björn Töpel <bjorn.topel@xxxxxxxxx>
> [...]
> > out_mmap_tx:
> > if (tx)
> > - munmap(xsk->tx,
> > - off.tx.desc +
> > + munmap(tx_map, off.tx.desc +
> > xsk->config.tx_size * sizeof(struct xdp_desc));
> > out_mmap_rx:
> > if (rx)
> > - munmap(xsk->rx,
> > - off.rx.desc +
> > + munmap(rx_map, off.rx.desc +
> > xsk->config.rx_size * sizeof(struct xdp_desc));
> > out_socket:
> > if (--umem->refcount)
> > @@ -684,10 +681,12 @@ int xsk_umem__delete(struct xsk_umem *umem)
> > optlen = sizeof(off);
> > err = getsockopt(umem->fd, SOL_XDP, XDP_MMAP_OFFSETS, &off, &optlen);
> > if (!err) {
> > - munmap(umem->fill->ring,
> > - off.fr.desc + umem->config.fill_size * sizeof(__u64));
> > - munmap(umem->comp->ring,
> > - off.cr.desc + umem->config.comp_size * sizeof(__u64));
> > + (void)munmap(umem->fill->ring - off.fr.desc,
> > + off.fr.desc +
> > + umem->config.fill_size * sizeof(__u64));
> > + (void)munmap(umem->comp->ring - off.cr.desc,
> > + off.cr.desc +
> > + umem->config.comp_size * sizeof(__u64));
>
> What's the rationale to cast to void here and other places (e.g. below)?
> If there's no proper reason, then lets remove it. Given the patch has already
> been applied, please send a follow up. Thanks.
>
The rationale was to make it explicit that the return value is *not*
cared about. If this is not common practice, I'll remove it in a
follow up!
Thank,
Björn
> > }
> >
> > close(umem->fd);
> > @@ -698,6 +697,7 @@ int xsk_umem__delete(struct xsk_umem *umem)
> >
> > void xsk_socket__delete(struct xsk_socket *xsk)
> > {
> > + size_t desc_sz = sizeof(struct xdp_desc);
> > struct xdp_mmap_offsets off;
> > socklen_t optlen;
> > int err;
> > @@ -710,14 +710,17 @@ void xsk_socket__delete(struct xsk_socket *xsk)
> > optlen = sizeof(off);
> > err = getsockopt(xsk->fd, SOL_XDP, XDP_MMAP_OFFSETS, &off, &optlen);
> > if (!err) {
> > - if (xsk->rx)
> > - munmap(xsk->rx->ring,
> > - off.rx.desc +
> > - xsk->config.rx_size * sizeof(struct xdp_desc));
> > - if (xsk->tx)
> > - munmap(xsk->tx->ring,
> > - off.tx.desc +
> > - xsk->config.tx_size * sizeof(struct xdp_desc));
> > + if (xsk->rx) {
> > + (void)munmap(xsk->rx->ring - off.rx.desc,
> > + off.rx.desc +
> > + xsk->config.rx_size * desc_sz);
> > + }
> > + if (xsk->tx) {
> > + (void)munmap(xsk->tx->ring - off.tx.desc,
> > + off.tx.desc +
> > + xsk->config.tx_size * desc_sz);
> > + }
> > +
> > }
> >
> > xsk->umem->refcount--;
> >
>
