Re: [PATCH bpf-next 4/4] bpf: inet_diag: Dump bpf_sk_storages in inet_diag_dump()

On Mon, Feb 24, 2020 at 09:47:33PM -0800, Song Liu wrote:

On Fri, Feb 21, 2020 at 10:49 AM Martin KaFai Lau [off-list ref] wrote:

quoted

This patch will dump out the bpf_sk_storages of a sk
if the request has the INET_DIAG_REQ_SK_BPF_STORAGES nlattr.

An array of SK_DIAG_BPF_STORAGE_REQ_MAP_FD can be specified in
INET_DIAG_REQ_SK_BPF_STORAGES to select which bpf_sk_storage to dump.
If no map_fd is specified, all bpf_sk_storages of a sk will be dumped.

[...]

quoted

Signed-off-by: Martin KaFai Lau <redacted>
---
 include/linux/inet_diag.h      |  4 ++
 include/linux/netlink.h        |  4 +-
 include/uapi/linux/inet_diag.h |  2 +
 net/ipv4/inet_diag.c           | 71 ++++++++++++++++++++++++++++++++++
 4 files changed, 79 insertions(+), 2 deletions(-)

diff --git a/include/linux/inet_diag.h b/include/linux/inet_diag.h
index 1bb94cac265f..e4ba25d63913 100644
--- a/include/linux/inet_diag.h
+++ b/include/linux/inet_diag.h

@@ -38,9 +38,13 @@ struct inet_diag_handler {
        __u16           idiag_info_size;
 };

+struct bpf_sk_storage_diag;
 struct inet_diag_dump_data {
        struct nlattr *req_nlas[__INET_DIAG_REQ_MAX];
 #define inet_diag_nla_bc req_nlas[INET_DIAG_REQ_BYTECODE]
+#define inet_diag_nla_bpf_stgs req_nlas[INET_DIAG_REQ_SK_BPF_STORAGES]
+
+       struct bpf_sk_storage_diag *bpf_stg_diag;
 };

 struct inet_connection_sock;

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 205fa7b1f07a..788969ccbbde 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h

@@ -188,10 +188,10 @@ struct netlink_callback {
        struct module           *module;
        struct netlink_ext_ack  *extack;
        u16                     family;
-       u16                     min_dump_alloc;
-       bool                    strict_check;
        u16                     answer_flags;
+       u32                     min_dump_alloc;

Maybe highlight this change in the commit log?

ok.

quoted

        unsigned int            prev_seq, seq;
+       bool                    strict_check;
        union {
                u8              ctx[48];

[ ... ]

quoted

diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 4bce8a477699..8ca4d54d7c5a 100644

[ ... ]

quoted

@@ -1022,8 +1069,11 @@ static int __inet_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
                            const struct inet_diag_req_v2 *r)
 {
        const struct inet_diag_handler *handler;
+       u32 prev_min_dump_alloc;
        int err = 0;

+again:
+       prev_min_dump_alloc = cb->min_dump_alloc;
        handler = inet_diag_lock_handler(r->sdiag_protocol);
        if (!IS_ERR(handler))
                handler->dump(skb, cb, r);

@@ -1031,6 +1081,12 @@ static int __inet_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
                err = PTR_ERR(handler);
        inet_diag_unlock_handler(handler);

+       if (!skb->len && cb->min_dump_alloc > prev_min_dump_alloc) {

Why do we check for !skb->len here?

skb contains the info of sk(s) to be dumped to the userspace.
It may contain no sk info (i.e. !skb->len),  1 sk info, 2 sk info...etc.
It only retries if there is no sk info and the cb->min_dump_alloc becomes
larger (together, it means the current skb is not large enough to fit one
sk info).

quoted

+               err = pskb_expand_head(skb, 0, cb->min_dump_alloc, GFP_KERNEL);
+               if (!err)
+                       goto again;
+       }
+
        return err ? : skb->len;
 }