| Message ID | 20210302023743.24123-9-xiyou.wangcong@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | sockmap: introduce BPF_SK_SKB_VERDICT and support UDP | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 8 maintainers not CCed: kuba@kernel.org yhs@fb.com ast@kernel.org kpsingh@kernel.org songliubraving@fb.com kafai@fb.com davem@davemloft.net andrii@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | fail | Errors and warnings before: 6 this patch: 6 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 11 lines checked |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 6 this patch: 6 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On 3/1/21 6:37 PM, Cong Wang wrote: > From: Cong Wang <cong.wang@bytedance.com> > > Now UDP supports sockmap and redirection, we can safely update > the sock type checks for it accordingly. > > Cc: John Fastabend <john.fastabend@gmail.com> > Cc: Daniel Borkmann <daniel@iogearbox.net> > Cc: Jakub Sitnicki <jakub@cloudflare.com> > Cc: Lorenz Bauer <lmb@cloudflare.com> > Signed-off-by: Cong Wang <cong.wang@bytedance.com> > --- > net/core/sock_map.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/net/core/sock_map.c b/net/core/sock_map.c > index 13d2af5bb81c..f7eee4b7b994 100644 > --- a/net/core/sock_map.c > +++ b/net/core/sock_map.c > @@ -549,7 +549,10 @@ static bool sk_is_udp(const struct sock *sk) > > static bool sock_map_redirect_allowed(const struct sock *sk) > { > - return sk_is_tcp(sk) && sk->sk_state != TCP_LISTEN; > + if (sk_is_tcp(sk)) > + return sk->sk_state != TCP_LISTEN; > + else > + return sk->sk_state == TCP_ESTABLISHED; Not a networking expert, a dump question. Here we tested whether sk_is_tcp(sk) or not, if not we compare sk->sk_state == TCP_ESTABLISHED, could this be always false? Mostly I missed something, some comments here will be good. > } > > static bool sock_map_sk_is_suitable(const struct sock *sk) >
On Tue, Mar 2, 2021 at 10:37 PM Yonghong Song <yhs@fb.com> wrote: > > > > On 3/1/21 6:37 PM, Cong Wang wrote: > > From: Cong Wang <cong.wang@bytedance.com> > > > > Now UDP supports sockmap and redirection, we can safely update > > the sock type checks for it accordingly. > > > > Cc: John Fastabend <john.fastabend@gmail.com> > > Cc: Daniel Borkmann <daniel@iogearbox.net> > > Cc: Jakub Sitnicki <jakub@cloudflare.com> > > Cc: Lorenz Bauer <lmb@cloudflare.com> > > Signed-off-by: Cong Wang <cong.wang@bytedance.com> > > --- > > net/core/sock_map.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/net/core/sock_map.c b/net/core/sock_map.c > > index 13d2af5bb81c..f7eee4b7b994 100644 > > --- a/net/core/sock_map.c > > +++ b/net/core/sock_map.c > > @@ -549,7 +549,10 @@ static bool sk_is_udp(const struct sock *sk) > > > > static bool sock_map_redirect_allowed(const struct sock *sk) > > { > > - return sk_is_tcp(sk) && sk->sk_state != TCP_LISTEN; > > + if (sk_is_tcp(sk)) > > + return sk->sk_state != TCP_LISTEN; > > + else > > + return sk->sk_state == TCP_ESTABLISHED; > > Not a networking expert, a dump question. Here we tested > whether sk_is_tcp(sk) or not, if not we compare > sk->sk_state == TCP_ESTABLISHED, could this be > always false? Mostly I missed something, some comments > here will be good. No, dgram sockets also use TCP_ESTABLISHED as a valid state. I know its name looks confusing, but it is already widely used in networking: net/appletalk/ddp.c: sk->sk_state = TCP_ESTABLISHED; net/appletalk/ddp.c: if (sk->sk_state != TCP_ESTABLISHED) net/appletalk/ddp.c: if (sk->sk_state != TCP_ESTABLISHED) net/ax25/af_ax25.c: sk->sk_state = TCP_ESTABLISHED; net/ax25/af_ax25.c: case TCP_ESTABLISHED: /* connection established */ net/ax25/af_ax25.c: if (sk->sk_state == TCP_ESTABLISHED && sk->sk_type == SOCK_SEQPACKET) { net/ax25/af_ax25.c: sk->sk_state = TCP_ESTABLISHED; net/ax25/af_ax25.c: if (sk->sk_state != TCP_ESTABLISHED && (flags & O_NONBLOCK)) { net/ax25/af_ax25.c: if (sk->sk_state != TCP_ESTABLISHED) { net/ax25/af_ax25.c: if (sk->sk_state != TCP_ESTABLISHED) { net/ax25/af_ax25.c: if (sk->sk_state != TCP_ESTABLISHED) { net/ax25/af_ax25.c: if (sk->sk_state != TCP_ESTABLISHED) { net/ax25/af_ax25.c: if (sk->sk_type == SOCK_SEQPACKET && sk->sk_state != TCP_ESTABLISHED) { net/ax25/ax25_ds_in.c: ax25->sk->sk_state = TCP_ESTABLISHED; net/ax25/ax25_in.c: make->sk_state = TCP_ESTABLISHED; net/ax25/ax25_std_in.c: ax25->sk->sk_state = TCP_ESTABLISHED; net/caif/caif_socket.c: CAIF_CONNECTED = TCP_ESTABLISHED, net/ceph/messenger.c: case TCP_ESTABLISHED: net/ceph/messenger.c: dout("%s TCP_ESTABLISHED\n", __func__); net/core/datagram.c: !(sk->sk_state == TCP_ESTABLISHED || sk->sk_state == TCP_LISTEN)) ... Hence, I believe it is okay to use it as it is, otherwise we would need to comment on every use of it. ;) Thanks.
On 3/3/21 10:02 AM, Cong Wang wrote: > On Tue, Mar 2, 2021 at 10:37 PM Yonghong Song <yhs@fb.com> wrote: >> >> >> >> On 3/1/21 6:37 PM, Cong Wang wrote: >>> From: Cong Wang <cong.wang@bytedance.com> >>> >>> Now UDP supports sockmap and redirection, we can safely update >>> the sock type checks for it accordingly. >>> >>> Cc: John Fastabend <john.fastabend@gmail.com> >>> Cc: Daniel Borkmann <daniel@iogearbox.net> >>> Cc: Jakub Sitnicki <jakub@cloudflare.com> >>> Cc: Lorenz Bauer <lmb@cloudflare.com> >>> Signed-off-by: Cong Wang <cong.wang@bytedance.com> >>> --- >>> net/core/sock_map.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/net/core/sock_map.c b/net/core/sock_map.c >>> index 13d2af5bb81c..f7eee4b7b994 100644 >>> --- a/net/core/sock_map.c >>> +++ b/net/core/sock_map.c >>> @@ -549,7 +549,10 @@ static bool sk_is_udp(const struct sock *sk) >>> >>> static bool sock_map_redirect_allowed(const struct sock *sk) >>> { >>> - return sk_is_tcp(sk) && sk->sk_state != TCP_LISTEN; >>> + if (sk_is_tcp(sk)) >>> + return sk->sk_state != TCP_LISTEN; >>> + else >>> + return sk->sk_state == TCP_ESTABLISHED; >> >> Not a networking expert, a dump question. Here we tested >> whether sk_is_tcp(sk) or not, if not we compare >> sk->sk_state == TCP_ESTABLISHED, could this be >> always false? Mostly I missed something, some comments >> here will be good. > > No, dgram sockets also use TCP_ESTABLISHED as a valid > state. I know its name looks confusing, but it is already widely > used in networking: > > net/appletalk/ddp.c: sk->sk_state = TCP_ESTABLISHED; > net/appletalk/ddp.c: if (sk->sk_state != TCP_ESTABLISHED) > net/appletalk/ddp.c: if (sk->sk_state != TCP_ESTABLISHED) > net/ax25/af_ax25.c: sk->sk_state = TCP_ESTABLISHED; > net/ax25/af_ax25.c: case TCP_ESTABLISHED: /* connection > established */ > net/ax25/af_ax25.c: if (sk->sk_state == TCP_ESTABLISHED && > sk->sk_type == SOCK_SEQPACKET) { > net/ax25/af_ax25.c: sk->sk_state = TCP_ESTABLISHED; > net/ax25/af_ax25.c: if (sk->sk_state != TCP_ESTABLISHED && (flags > & O_NONBLOCK)) { > net/ax25/af_ax25.c: if (sk->sk_state != TCP_ESTABLISHED) { > net/ax25/af_ax25.c: if (sk->sk_state != TCP_ESTABLISHED) { > net/ax25/af_ax25.c: if (sk->sk_state != TCP_ESTABLISHED) { > net/ax25/af_ax25.c: if (sk->sk_state != TCP_ESTABLISHED) { > net/ax25/af_ax25.c: if (sk->sk_type == SOCK_SEQPACKET && > sk->sk_state != TCP_ESTABLISHED) { > net/ax25/ax25_ds_in.c: ax25->sk->sk_state = TCP_ESTABLISHED; > net/ax25/ax25_in.c: make->sk_state = TCP_ESTABLISHED; > net/ax25/ax25_std_in.c: ax25->sk->sk_state = > TCP_ESTABLISHED; > net/caif/caif_socket.c: CAIF_CONNECTED = TCP_ESTABLISHED, > net/ceph/messenger.c: case TCP_ESTABLISHED: > net/ceph/messenger.c: dout("%s TCP_ESTABLISHED\n", __func__); > net/core/datagram.c: !(sk->sk_state == TCP_ESTABLISHED || > sk->sk_state == TCP_LISTEN)) > ... > > Hence, I believe it is okay to use it as it is, otherwise we would need > to comment on every use of it. ;) That is okay. Thanks for explanation! > > Thanks. >
diff --git a/net/core/sock_map.c b/net/core/sock_map.c index 13d2af5bb81c..f7eee4b7b994 100644 --- a/net/core/sock_map.c +++ b/net/core/sock_map.c @@ -549,7 +549,10 @@ static bool sk_is_udp(const struct sock *sk) static bool sock_map_redirect_allowed(const struct sock *sk) { - return sk_is_tcp(sk) && sk->sk_state != TCP_LISTEN; + if (sk_is_tcp(sk)) + return sk->sk_state != TCP_LISTEN; + else + return sk->sk_state == TCP_ESTABLISHED; } static bool sock_map_sk_is_suitable(const struct sock *sk)