The type of sk_rcvbuf and sk_sndbuf in struct sock is int, and
in tcp_add_backlog(), the variable limit is caculated by adding
sk_rcvbuf, sk_sndbuf and 64 * 1024, it may exceed the max value
of u32 and be truncated. So change it to u64 to avoid a potential
signed-integer-overflow, which leads to opposite result is returned
in the following function.
Signed-off-by: Lu Wei <[email protected]>
---
include/net/sock.h | 4 ++--
net/ipv4/tcp_ipv4.c | 6 ++++--
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index 08038a385ef2..fc0fa29d8865 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1069,7 +1069,7 @@ static inline void __sk_add_backlog(struct sock *sk, struct sk_buff *skb)
* Do not take into account this skb truesize,
* to allow even a single big packet to come.
*/
-static inline bool sk_rcvqueues_full(const struct sock *sk, unsigned int limit)
+static inline bool sk_rcvqueues_full(const struct sock *sk, u64 limit)
{
unsigned int qsize = sk->sk_backlog.len + atomic_read(&sk->sk_rmem_alloc);
@@ -1078,7 +1078,7 @@ static inline bool sk_rcvqueues_full(const struct sock *sk, unsigned int limit)
/* The per-socket spinlock must be held here. */
static inline __must_check int sk_add_backlog(struct sock *sk, struct sk_buff *skb,
- unsigned int limit)
+ u64 limit)
{
if (sk_rcvqueues_full(sk, limit))
return -ENOBUFS;
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 6376ad915765..3d4f9ac64165 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1769,7 +1769,8 @@ int tcp_v4_early_demux(struct sk_buff *skb)
bool tcp_add_backlog(struct sock *sk, struct sk_buff *skb,
enum skb_drop_reason *reason)
{
- u32 limit, tail_gso_size, tail_gso_segs;
+ u32 tail_gso_size, tail_gso_segs;
+ u64 limit;
struct skb_shared_info *shinfo;
const struct tcphdr *th;
struct tcphdr *thtail;
@@ -1878,7 +1879,8 @@ bool tcp_add_backlog(struct sock *sk, struct sk_buff *skb,
* to reduce memory overhead, so add a little headroom here.
* Few sockets backlog are possibly concurrently non empty.
*/
- limit = READ_ONCE(sk->sk_rcvbuf) + READ_ONCE(sk->sk_sndbuf) + 64*1024;
+ limit = (u64)READ_ONCE(sk->sk_rcvbuf) +
+ (u64)READ_ONCE(sk->sk_sndbuf) + 64*1024;
if (unlikely(sk_add_backlog(sk, skb, limit))) {
bh_unlock_sock(sk);
--
2.31.1
On Wed, Oct 12, 2022 at 2:35 AM Lu Wei <[email protected]> wrote:
>
> The type of sk_rcvbuf and sk_sndbuf in struct sock is int, and
> in tcp_add_backlog(), the variable limit is caculated by adding
> sk_rcvbuf, sk_sndbuf and 64 * 1024, it may exceed the max value
> of u32 and be truncated. So change it to u64 to avoid a potential
> signed-integer-overflow, which leads to opposite result is returned
> in the following function.
>
> Signed-off-by: Lu Wei <[email protected]>
You need to add a Fixes: tag, please.
> ---
> include/net/sock.h | 4 ++--
> net/ipv4/tcp_ipv4.c | 6 ++++--
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 08038a385ef2..fc0fa29d8865 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1069,7 +1069,7 @@ static inline void __sk_add_backlog(struct sock *sk, struct sk_buff *skb)
> * Do not take into account this skb truesize,
> * to allow even a single big packet to come.
> */
> -static inline bool sk_rcvqueues_full(const struct sock *sk, unsigned int limit)
> +static inline bool sk_rcvqueues_full(const struct sock *sk, u64 limit)
> {
> unsigned int qsize = sk->sk_backlog.len + atomic_read(&sk->sk_rmem_alloc);
qsize would then overflow :/
I would rather limit sk_rcvbuf and sk_sndbuf to 0x7fff0000, instead of
0x7ffffffe
If really someone is using 2GB for both send and receive queues, I
doubt removing 64KB will be a problem.
在 2022/10/12 8:31 PM, Eric Dumazet 写道:
> On Wed, Oct 12, 2022 at 2:35 AM Lu Wei <[email protected]> wrote:
>> The type of sk_rcvbuf and sk_sndbuf in struct sock is int, and
>> in tcp_add_backlog(), the variable limit is caculated by adding
>> sk_rcvbuf, sk_sndbuf and 64 * 1024, it may exceed the max value
>> of u32 and be truncated. So change it to u64 to avoid a potential
>> signed-integer-overflow, which leads to opposite result is returned
>> in the following function.
>>
>> Signed-off-by: Lu Wei <[email protected]>
> You need to add a Fixes: tag, please.
>
>> ---
>> include/net/sock.h | 4 ++--
>> net/ipv4/tcp_ipv4.c | 6 ++++--
>> 2 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/net/sock.h b/include/net/sock.h
>> index 08038a385ef2..fc0fa29d8865 100644
>> --- a/include/net/sock.h
>> +++ b/include/net/sock.h
>> @@ -1069,7 +1069,7 @@ static inline void __sk_add_backlog(struct sock *sk, struct sk_buff *skb)
>> * Do not take into account this skb truesize,
>> * to allow even a single big packet to come.
>> */
>> -static inline bool sk_rcvqueues_full(const struct sock *sk, unsigned int limit)
>> +static inline bool sk_rcvqueues_full(const struct sock *sk, u64 limit)
>> {
>> unsigned int qsize = sk->sk_backlog.len + atomic_read(&sk->sk_rmem_alloc);
> qsize would then overflow :/
>
> I would rather limit sk_rcvbuf and sk_sndbuf to 0x7fff0000, instead of
> 0x7ffffffe
>
> If really someone is using 2GB for both send and receive queues, I
> doubt removing 64KB will be a problem.
> .
thanks for reply, I will change the type of qsize to u64 in V2. Besides,
how to limit sk_rcvbuf and sk_sndbuf
to 0x7ffff0000, do you mean in sysctl interface? If so, the varible
limit will still overflow since it's calculated
by adding sk_rcvbuf and sk_sndbuf.
--
Best Regards,
Lu Wei
On Tue, Oct 18, 2022 at 12:45 AM luwei (O) <[email protected]> wrote:
>
>
> 在 2022/10/12 8:31 PM, Eric Dumazet 写道:
> > On Wed, Oct 12, 2022 at 2:35 AM Lu Wei <[email protected]> wrote:
> >> The type of sk_rcvbuf and sk_sndbuf in struct sock is int, and
> >> in tcp_add_backlog(), the variable limit is caculated by adding
> >> sk_rcvbuf, sk_sndbuf and 64 * 1024, it may exceed the max value
> >> of u32 and be truncated. So change it to u64 to avoid a potential
> >> signed-integer-overflow, which leads to opposite result is returned
> >> in the following function.
> >>
> >> Signed-off-by: Lu Wei <[email protected]>
> > You need to add a Fixes: tag, please.
> >
> >> ---
> >> include/net/sock.h | 4 ++--
> >> net/ipv4/tcp_ipv4.c | 6 ++++--
> >> 2 files changed, 6 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/include/net/sock.h b/include/net/sock.h
> >> index 08038a385ef2..fc0fa29d8865 100644
> >> --- a/include/net/sock.h
> >> +++ b/include/net/sock.h
> >> @@ -1069,7 +1069,7 @@ static inline void __sk_add_backlog(struct sock *sk, struct sk_buff *skb)
> >> * Do not take into account this skb truesize,
> >> * to allow even a single big packet to come.
> >> */
> >> -static inline bool sk_rcvqueues_full(const struct sock *sk, unsigned int limit)
> >> +static inline bool sk_rcvqueues_full(const struct sock *sk, u64 limit)
> >> {
> >> unsigned int qsize = sk->sk_backlog.len + atomic_read(&sk->sk_rmem_alloc);
> > qsize would then overflow :/
> >
> > I would rather limit sk_rcvbuf and sk_sndbuf to 0x7fff0000, instead of
> > 0x7ffffffe
> >
> > If really someone is using 2GB for both send and receive queues, I
> > doubt removing 64KB will be a problem.
> > .
>
> thanks for reply, I will change the type of qsize to u64 in V2. Besides,
> how to limit sk_rcvbuf and sk_sndbuf
Please do not add u64 where not really needed.
TCP stack is not ready for huge queues, we still have O(N)
pathological functions,
especially when dealing with memory pressure.
Unless you want to solve this difficult problem, let's not send wrong signals.
>
> to 0x7ffff0000, do you mean in sysctl interface? If so, the varible
> limit will still overflow since it's calculated
>
> by adding sk_rcvbuf and sk_sndbuf.
u32 limit = (u32) rcvbuf + (u32) sndbuf + 64*1024; does not overflow.
0x7fff0000U + 0x7fff0000U + 0x10000 = 0xffff0000
>
> --
> Best Regards,
> Lu Wei
>