From: Lawrence Brakmo <brakmo@fb.com>
To: Eric Dumazet <eric.dumazet@gmail.com>, netdev <netdev@vger.kernel.org>
Cc: Kernel Team <Kernel-team@fb.com>, Blake Matheny <bmatheny@fb.com>,
"Alexei Starovoitov" <ast@fb.com>,
Daniel Borkmann <daniel@iogearbox.net>,
"Neal Cardwell" <ncardwell@google.com>,
Yuchung Cheng <ycheng@google.com>
Subject: Re: [PATCH bpf-next v5 03/11] bpf: Add write access to tcp_sock and sock fields
Date: Tue, 9 Jan 2018 23:41:31 +0000 [thread overview]
Message-ID: <26D03FD3-CA3C-404A-AEEF-D87CC2BEF886@fb.com> (raw)
In-Reply-To: <1515540108.131759.13.camel@gmail.com>
On 1/9/18, 3:22 PM, "netdev-owner@vger.kernel.org on behalf of Eric Dumazet" <netdev-owner@vger.kernel.org on behalf of eric.dumazet@gmail.com> wrote:
On Tue, 2018-01-09 at 13:06 -0800, Lawrence Brakmo wrote:
> This patch adds a macro, SOCK_OPS_SET_FIELD, for writing to
> struct tcp_sock or struct sock fields. This required adding a new
> field "temp" to struct bpf_sock_ops_kern for temporary storage that
> is used by sock_ops_convert_ctx_access. It is used to store and recover
> the contents of a register, so the register can be used to store the
> address of the sk. Since we cannot overwrite the dst_reg because it
> contains the pointer to ctx, nor the src_reg since it contains the value
> we want to store, we need an extra register to contain the address
> of the sk.
>
> Also adds the macro SOCK_OPS_GET_OR_SET_FIELD that calls one of the
> GET or SET macros depending on the value of the TYPE field.
>
> Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
> ---
> include/linux/filter.h | 3 +++
> include/net/tcp.h | 2 +-
> net/core/filter.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 425056c..139a9a3 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -1007,6 +1007,9 @@ struct bpf_sock_ops_kern {
> u32 replylong[4];
> };
> u32 is_fullsock;
> + u64 temp; /* Used by sock_ops_convert_ctx_access
> + * as temporary storaage of a register
typo on storage
Thanks, will fix.
> + */
> };
>
> #endif /* __LINUX_FILTER_H__ */
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 6939e69..108d16a 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -2010,7 +2010,7 @@ static inline int tcp_call_bpf(struct sock *sk, int op)
> struct bpf_sock_ops_kern sock_ops;
> int ret;
>
> - memset(&sock_ops, 0, sizeof(sock_ops));
> + memset(&sock_ops, 0, offsetof(struct bpf_sock_ops_kern, temp));
>
This lacks proper comment, stating that every field added after .temp
in struct bpf_sock_ops_kern is considered as scratch and could contain
garbage.
I assumed that one would modify this initialization appropriately when adding
a new field. But it would certainly make sense to indicate that new, non-scratch,
fields should be added before temp so there would be no need to modify this
initialization code. Makes sense?
next prev parent reply other threads:[~2018-01-09 23:41 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-09 21:06 [PATCH bpf-next v5 00/11] bpf: More sock_ops callbacks Lawrence Brakmo
2018-01-09 21:06 ` [PATCH bpf-next v5 01/11] bpf: Make SOCK_OPS_GET_TCP size independent Lawrence Brakmo
2018-01-09 21:06 ` [PATCH bpf-next v5 02/11] bpf: Make SOCK_OPS_GET_TCP struct independent Lawrence Brakmo
2018-01-09 21:06 ` [PATCH bpf-next v5 03/11] bpf: Add write access to tcp_sock and sock fields Lawrence Brakmo
2018-01-09 23:21 ` Eric Dumazet
2018-01-09 23:41 ` Lawrence Brakmo [this message]
2018-01-09 21:06 ` [PATCH bpf-next v5 04/11] bpf: Support passing args to sock_ops bpf function Lawrence Brakmo
2018-01-09 21:06 ` [PATCH bpf-next v5 05/11] bpf: Adds field bpf_sock_ops_flags to tcp_sock Lawrence Brakmo
2018-01-09 23:30 ` Eric Dumazet
2018-01-10 0:31 ` Lawrence Brakmo
2018-01-09 21:06 ` [PATCH bpf-next v5 06/11] bpf: Add sock_ops RTO callback Lawrence Brakmo
2018-01-09 21:07 ` [PATCH bpf-next v5 07/11] bpf: Add support for reading sk_state and more Lawrence Brakmo
2018-01-09 21:07 ` [PATCH bpf-next v5 08/11] bpf: Add sock_ops R/W access to tclass & sk_txhash Lawrence Brakmo
2018-01-09 21:07 ` [PATCH bpf-next v5 09/11] bpf: Add BPF_SOCK_OPS_RETRANS_CB Lawrence Brakmo
2018-01-09 21:07 ` [PATCH bpf-next v5 10/11] bpf: Add BPF_SOCK_OPS_STATE_CB Lawrence Brakmo
2018-01-09 21:07 ` [PATCH bpf-next v5 11/11] bpf: add selftest for tcpbpf Lawrence Brakmo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=26D03FD3-CA3C-404A-AEEF-D87CC2BEF886@fb.com \
--to=brakmo@fb.com \
--cc=Kernel-team@fb.com \
--cc=ast@fb.com \
--cc=bmatheny@fb.com \
--cc=daniel@iogearbox.net \
--cc=eric.dumazet@gmail.com \
--cc=ncardwell@google.com \
--cc=netdev@vger.kernel.org \
--cc=ycheng@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).