Re: [PATCH bpf-next v3] docs/bpf: Add documentation for BPF_MAP_TYPE_SK_STORAGE

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 12/8/22 8:35 AM, Donald Hunter wrote:
Donald Hunter <donald.hunter@xxxxxxxxx> writes:

David Vernet <void@xxxxxxxxxxxxx> writes:

On Wed, Dec 07, 2022 at 10:27:21AM +0000, Donald Hunter wrote:
+
+This snippet shows how to retrieve socket-local storage in a BPF program:
+
+.. code-block:: c
+
+    SEC("sockops")
+    int _sockops(struct bpf_sock_ops *ctx)
+    {
+            struct my_storage *storage;
+            struct bpf_sock *sk;
+
+            sk = ctx->sk;
+            if (!sk)
+                    return 1;

Don't feel strongly about this one, but IMO it's nice for examples to
illustrate code that's as close to real and pristine as possible. To
that point, should this example perhaps be updated to return -ENOENT
here, and -ENOMEM below?

Will do.


After digging into this a bit more I notice that the sockops programs in
tools/testing/selftests/bpf/progs mostly return 1 in all cases.

I'm assuming that sockops programs should return valid values for
some op types such as BPF_SOCK_OPS_TIMEOUT_INIT. Other than that I can't
find a definitive list. Do you know if valid return values are
enumerated anywhere, or do I need to dig some more?

It can return any integer.

static inline u32 tcp_timeout_init(struct sock *sk)
{
        int timeout;

        timeout = tcp_call_bpf(sk, BPF_SOCK_OPS_TIMEOUT_INIT, 0, NULL);

        if (timeout <= 0)
                timeout = TCP_TIMEOUT_INIT;
        return min_t(int, timeout, TCP_RTO_MAX);
}

In uapi bpf.h,

BPF_SOCK_OPS_TIMEOUT_INIT, /* Should return SYN-RTO value to use or * -1 if default value should be used
                                         */

I think the above code is from selftests tcp_rtt.c. You can add a
reference to provide more context. If people are really interested,
they can go to selftest to find more.


+
+            storage = bpf_sk_storage_get(&socket_storage, sk, 0,
+                                         BPF_LOCAL_STORAGE_GET_F_CREATE);
+            if (!storage)
+                    return 1;
+
+            /* Use 'storage' here */

Let's return 0 at the end to make the example program technically
correct.

Will do.

In this case, 'return 0' probably not correct. I suggest keep the
code as is to sync with selftests. Add a reference to selftest
for more reference.


+    }

Thanks,
David



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux