Re: [PATCH bpf-next 3/3] xsk: add sock_diag interface for AF_XDP

[Daniel Borkmann <daniel@iogearbox.net>]

On 01/23/2019 03:24 PM, Björn Töpel wrote:
> Den ons 23 jan. 2019 kl 14:19 skrev Daniel Borkmann <daniel@iogearbox.net>:
>>
>> On 01/18/2019 02:03 PM, bjorn.topel@gmail.com wrote:
>>> From: Björn Töpel <bjorn.topel@intel.com>
>>>
>>> This patch adds the sock_diag interface for querying sockets from user
>>> space. Tools like iproute2 ss(8) can use this interface to list open
>>> AF_XDP sockets.
>>>
>>> The user-space ABI is defined in linux/xdp_diag.h and includes netlink
>>> request and response structs. The request can query sockets and the
>>> response contains socket information about the rings, umems, inode and
>>> more.
>>>
>>> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
>>
>> Series looks good, few minor nits inline:
>>
>>> ---
>>>  include/uapi/linux/xdp_diag.h |  72 +++++++++++++
>>>  net/xdp/Kconfig               |   8 ++
>>>  net/xdp/Makefile              |   1 +
>>>  net/xdp/xsk.c                 |   6 +-
>>>  net/xdp/xsk.h                 |  12 +++
>>>  net/xdp/xsk_diag.c            | 192 ++++++++++++++++++++++++++++++++++
>>>  6 files changed, 286 insertions(+), 5 deletions(-)
>>>  create mode 100644 include/uapi/linux/xdp_diag.h
>>>  create mode 100644 net/xdp/xsk.h
>>>  create mode 100644 net/xdp/xsk_diag.c
>>>
>>> diff --git a/include/uapi/linux/xdp_diag.h b/include/uapi/linux/xdp_diag.h
>>> new file mode 100644
>>> index 000000000000..efe8ce281dce
>>> --- /dev/null
>>> +++ b/include/uapi/linux/xdp_diag.h
>>> @@ -0,0 +1,72 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>>> +/*
>>> + * xdp_diag: interface for query/monitor XDP sockets
>>> + * Copyright(c) 2019 Intel Corporation.
>>> + */
>>> +
>>> +#ifndef _LINUX_XDP_DIAG_H
>>> +#define _LINUX_XDP_DIAG_H
>>> +
>>> +#include <linux/types.h>
>>> +
>>> +struct xdp_diag_req {
>>> +     __u8    sdiag_family;
>>> +     __u8    sdiag_protocol;
>>> +     __u16   pad;
>>
>> Presumably this one is for future use? Maybe better as '__u16 :16;' to
>> avoid compile errors if someone tries to zero 'pad' member manually?
> 
> The "pad" was there simply to have an explicitly named structure hole.
> I'm not following the bitfield argument. Why does that avoid compiler
> errors?

Mostly in the sense that an application would explicitly set 'pad = 0'
whereas pad could later on potentially be renamed and reused otherwise
(which __u16 :16 would avoid in first place). But looking at other
*_diag_req structs, it's explicitly named as 'pad' elsewhere already,
then nevermind, lets have it rather consistent then and keep as is.

One small thing I still spotted when looking at it again, in function
xsk_diag_handler_dump() the req is unused.

Thanks,
Daniel