| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANn89iK40SoyJ8fS2U5kp3pDruo=zfQNPL-ppOF+LYaS9z-MVA@mail.gmail.com>
Date: Fri, 9 Feb 2024 12:01:46 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Jason Xing <kerneljasonxing@...il.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
dsahern@...nel.org, netdev@...r.kernel.org,
Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH v2 net-next 2/2] tcp: add more DROP REASONs in receive process
On Fri, Feb 9, 2024 at 7:12 AM Jason Xing <kerneljasonxing@...il.com> wrote:
>
> From: Jason Xing <kernelxing@...cent.com>
>
> As the title said, add more reasons to narrow down the range about
> why the skb should be dropped.
>
> Signed-off-by: Jason Xing <kernelxing@...cent.com>
> ---
> include/net/dropreason-core.h | 11 ++++++++++-
> include/net/tcp.h | 4 ++--
> net/ipv4/tcp_input.c | 26 +++++++++++++++++---------
> net/ipv4/tcp_ipv4.c | 19 ++++++++++++-------
> net/ipv4/tcp_minisocks.c | 10 +++++-----
> net/ipv6/tcp_ipv6.c | 19 ++++++++++++-------
> 6 files changed, 58 insertions(+), 31 deletions(-)
>
> diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
> index efbc5dfd9e84..9a7643be9d07 100644
> --- a/include/net/dropreason-core.h
> +++ b/include/net/dropreason-core.h
> @@ -31,6 +31,8 @@
> FN(TCP_AOFAILURE) \
> FN(SOCKET_BACKLOG) \
> FN(TCP_FLAGS) \
> + FN(TCP_CONNREQNOTACCEPTABLE) \
> + FN(TCP_ABORTONDATA) \
> FN(TCP_ZEROWINDOW) \
> FN(TCP_OLD_DATA) \
> FN(TCP_OVERWINDOW) \
> @@ -38,6 +40,7 @@
> FN(TCP_RFC7323_PAWS) \
> FN(TCP_OLD_SEQUENCE) \
> FN(TCP_INVALID_SEQUENCE) \
> + FN(TCP_INVALID_ACK_SEQUENCE) \
> FN(TCP_RESET) \
> FN(TCP_INVALID_SYN) \
> FN(TCP_CLOSE) \
> @@ -203,6 +206,10 @@ enum skb_drop_reason {
> SKB_DROP_REASON_SOCKET_BACKLOG,
> /** @SKB_DROP_REASON_TCP_FLAGS: TCP flags invalid */
> SKB_DROP_REASON_TCP_FLAGS,
> + /** @SKB_DROP_REASON_TCP_CONNREQNOTACCEPTABLE: con req not acceptable */
> + SKB_DROP_REASON_TCP_CONNREQNOTACCEPTABLE,
> + /** @SKB_DROP_REASON_TCP_ABORTONDATA: abort on data */
> + SKB_DROP_REASON_TCP_ABORTONDATA,
> /**
> * @SKB_DROP_REASON_TCP_ZEROWINDOW: TCP receive window size is zero,
> * see LINUX_MIB_TCPZEROWINDOWDROP
> @@ -227,13 +234,15 @@ enum skb_drop_reason {
> SKB_DROP_REASON_TCP_OFOMERGE,
> /**
> * @SKB_DROP_REASON_TCP_RFC7323_PAWS: PAWS check, corresponding to
> - * LINUX_MIB_PAWSESTABREJECTED
> + * LINUX_MIB_PAWSESTABREJECTED, LINUX_MIB_PAWSACTIVEREJECTED
> */
> SKB_DROP_REASON_TCP_RFC7323_PAWS,
> /** @SKB_DROP_REASON_TCP_OLD_SEQUENCE: Old SEQ field (duplicate packet) */
> SKB_DROP_REASON_TCP_OLD_SEQUENCE,
> /** @SKB_DROP_REASON_TCP_INVALID_SEQUENCE: Not acceptable SEQ field */
> SKB_DROP_REASON_TCP_INVALID_SEQUENCE,
> + /** @SKB_DROP_REASON_TCP_ACK_INVALID_SEQUENCE: Not acceptable ACK SEQ field */
> + SKB_DROP_REASON_TCP_INVALID_ACK_SEQUENCE,
> /** @SKB_DROP_REASON_TCP_RESET: Invalid RST packet */
> SKB_DROP_REASON_TCP_RESET,
> /**
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 58e65af74ad1..1d9b2a766b5e 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -348,7 +348,7 @@ void tcp_wfree(struct sk_buff *skb);
> void tcp_write_timer_handler(struct sock *sk);
> void tcp_delack_timer_handler(struct sock *sk);
> int tcp_ioctl(struct sock *sk, int cmd, int *karg);
> -int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb);
> +enum skb_drop_reason tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb);
> void tcp_rcv_established(struct sock *sk, struct sk_buff *skb);
> void tcp_rcv_space_adjust(struct sock *sk);
> int tcp_twsk_unique(struct sock *sk, struct sock *sktw, void *twp);
> @@ -396,7 +396,7 @@ enum tcp_tw_status tcp_timewait_state_process(struct inet_timewait_sock *tw,
> struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
> struct request_sock *req, bool fastopen,
> bool *lost_race);
> -int tcp_child_process(struct sock *parent, struct sock *child,
> +enum skb_drop_reason tcp_child_process(struct sock *parent, struct sock *child,
> struct sk_buff *skb);
> void tcp_enter_loss(struct sock *sk);
> void tcp_cwnd_reduction(struct sock *sk, int newly_acked_sacked, int newly_lost, int flag);
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 2d20edf652e6..81fe584aa777 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -6361,6 +6361,7 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
> inet_csk_reset_xmit_timer(sk,
> ICSK_TIME_RETRANS,
> TCP_TIMEOUT_MIN, TCP_RTO_MAX);
> + SKB_DR_SET(reason, TCP_INVALID_ACK_SEQUENCE);
> goto reset_and_undo;
> }
>
> @@ -6369,6 +6370,7 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
> tcp_time_stamp_ts(tp))) {
> NET_INC_STATS(sock_net(sk),
> LINUX_MIB_PAWSACTIVEREJECTED);
> + SKB_DR_SET(reason, TCP_RFC7323_PAWS);
> goto reset_and_undo;
> }
>
> @@ -6572,7 +6574,7 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
> reset_and_undo:
> tcp_clear_options(&tp->rx_opt);
> tp->rx_opt.mss_clamp = saved_clamp;
> - return 1;
> + return reason;
> }
>
> static void tcp_rcv_synrecv_state_fastopen(struct sock *sk)
> @@ -6616,7 +6618,8 @@ static void tcp_rcv_synrecv_state_fastopen(struct sock *sk)
> * address independent.
> */
>
> -int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
> +enum skb_drop_reason
> +tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
> {
> struct tcp_sock *tp = tcp_sk(sk);
> struct inet_connection_sock *icsk = inet_csk(sk);
> @@ -6633,7 +6636,7 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
>
> case TCP_LISTEN:
> if (th->ack)
> - return 1;
> + return SKB_DROP_REASON_TCP_FLAGS;
>
> if (th->rst) {
> SKB_DR_SET(reason, TCP_RESET);
> @@ -6654,7 +6657,7 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
> rcu_read_unlock();
>
> if (!acceptable)
> - return 1;
> + return SKB_DROP_REASON_TCP_CONNREQNOTACCEPTABLE;
> consume_skb(skb);
> return 0;
> }
> @@ -6704,8 +6707,13 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
> FLAG_NO_CHALLENGE_ACK);
>
> if ((int)reason <= 0) {
> - if (sk->sk_state == TCP_SYN_RECV)
> - return 1; /* send one RST */
> + if (sk->sk_state == TCP_SYN_RECV) {
> + /* send one RST */
> + if (!reason)
> + return SKB_DROP_REASON_TCP_OLD_ACK;
> + else
> + return -reason;
Your patch is too large/risky, not that I am trying to be gentle here...
You should know better that we are not going to accept it as is.
Please split your patches into smaller ones.
Eg, a single patch to change tcp_rcv_synsent_state_process() return value.
It used to return -1, 0, or 1.
Take the time to document for tcp_rcv_synsent_state_process() what are
the new possible return values.
Then other changes, one at a time, in a logical way.
Smaller patches are easier to review, even if it forces the author to
think a bit more about how to
make his series a work of art. Everyone wins, because we spend less
time and we learn faster.
Thank you.
Powered by blists - more mailing lists