On 05/06/2019 10:40 AM, Björn Töpel wrote:
> 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!
Yeah, it's not common practice, so lets rather remove it.
Thanks,
Daniel
