Re: [PATCH bpf 1/2] libbpf: fix invalid munmap call

[Daniel Borkmann <daniel@iogearbox.net>]

On 05/06/2019 10:40 AM, Björn Töpel wrote:
> On Mon, 6 May 2019 at 10:26, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 04/30/2019 02:45 PM, Björn Töpel wrote:
>>> From: Björn Töpel <bjorn.topel@intel.com>
>>>
>>> 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@gmail.com>
>>> Fixes: 1cad07884239 ("libbpf: add support for using AF_XDP sockets")
>>> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
>> [...]
>>>  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!

Yeah, it's not common practice, so lets rather remove it.

Thanks,
Daniel