On Mon, 6 May 2019 at 10:26, Daniel Borkmann [off-list ref] wrote:

On 04/30/2019 02:45 PM, Björn Töpel wrote:

quoted

From: Björn Töpel <redacted>

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 <redacted>
Fixes: 1cad07884239 ("libbpf: add support for using AF_XDP sockets")
Signed-off-by: Björn Töpel <redacted>

[...]

quoted

 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

quoted

      }

      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--;