| Submitter | Peter Zijlstra |
|---|---|
| Date | Oct. 2, 2008, 1:05 p.m. |
| Message ID | <20081002131608.821584767@chello.nl> |
| Download | mbox | patch |
| Permalink | /patch/2384/ |
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
Comments
From: Peter Zijlstra <a.p.zijlstra@chello.nl> Date: Thu, 02 Oct 2008 15:05:22 +0200 > @@ -952,6 +955,7 @@ static void tcp_v6_send_reset(struct soc > #ifdef CONFIG_TCP_MD5SIG > struct tcp_md5sig_key *key; > #endif > + gfp_t gfp_mask = GFP_ATOMIC; > > if (th->rst) > return; > @@ -969,13 +973,16 @@ static void tcp_v6_send_reset(struct soc > tot_len += TCPOLEN_MD5SIG_ALIGNED; > #endif > > + if (sk) > + gfp_mask = sk_allocation(skb->sk, gfp_mask); > + > /* > * We need to grab some memory, and put together an RST, > * and then put it into the queue to be sent. > */ > > buff = alloc_skb(MAX_HEADER + sizeof(struct ipv6hdr) + tot_len, > - GFP_ATOMIC); > + sk_allocation(sk, GFP_ATOMIC)); > if (buff == NULL) > return; > I don't think this is doing what you intend it to do. First, you're conditionally calling sk_allocation() if 'sk' is non-NULL. But then later you unconditionally use sk_allocation() in the alloc_skb() call. Furthermore, in the conditionalized case you're using "skb->sk" instead of plain "sk" which is what you actually checked against NULL. I have no fundamental problem with this change, so please audit this patch for similar problems, fix them all up, and resubmit. I'm also tossing the rest of your networking changes since they'll have some dependency on this one, please resend those at the same time as the fixed up version of this one. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2008-10-07 at 14:26 -0700, David Miller wrote: > From: Peter Zijlstra <a.p.zijlstra@chello.nl> > Date: Thu, 02 Oct 2008 15:05:22 +0200 > > > @@ -952,6 +955,7 @@ static void tcp_v6_send_reset(struct soc > > #ifdef CONFIG_TCP_MD5SIG > > struct tcp_md5sig_key *key; > > #endif > > + gfp_t gfp_mask = GFP_ATOMIC; > > > > if (th->rst) > > return; > > @@ -969,13 +973,16 @@ static void tcp_v6_send_reset(struct soc > > tot_len += TCPOLEN_MD5SIG_ALIGNED; > > #endif > > > > + if (sk) > > + gfp_mask = sk_allocation(skb->sk, gfp_mask); > > + > > /* > > * We need to grab some memory, and put together an RST, > > * and then put it into the queue to be sent. > > */ > > > > buff = alloc_skb(MAX_HEADER + sizeof(struct ipv6hdr) + tot_len, > > - GFP_ATOMIC); > > + sk_allocation(sk, GFP_ATOMIC)); > > if (buff == NULL) > > return; > > > > I don't think this is doing what you intend it to do. > > First, you're conditionally calling sk_allocation() if > 'sk' is non-NULL. But then later you unconditionally > use sk_allocation() in the alloc_skb() call. > > Furthermore, in the conditionalized case you're using > "skb->sk" instead of plain "sk" which is what you actually > checked against NULL. > > I have no fundamental problem with this change, so please > audit this patch for similar problems, fix them all up, > and resubmit. > > I'm also tossing the rest of your networking changes since > they'll have some dependency on this one, please resend those > at the same time as the fixed up version of this one. You're right - Suresh Jayaraman hit an oops here, just fixing up that obviously mis-merged conditional didn't fix it for him. So I'll work on fixing this for him. Then I'll make a new patch-series against linux-next and include the driver parts you left out of 17. Thanks! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Patch
Index: linux-2.6/net/ipv4/tcp_output.c =================================================================== --- linux-2.6.orig/net/ipv4/tcp_output.c +++ linux-2.6/net/ipv4/tcp_output.c @@ -2148,7 +2148,8 @@ void tcp_send_fin(struct sock *sk) } else { /* Socket is locked, keep trying until memory is available. */ for (;;) { - skb = alloc_skb_fclone(MAX_TCP_HEADER, GFP_KERNEL); + skb = alloc_skb_fclone(MAX_TCP_HEADER, + sk_allocation(sk, GFP_KERNEL)); if (skb) break; yield(); @@ -2174,7 +2175,7 @@ void tcp_send_active_reset(struct sock * struct sk_buff *skb; /* NOTE: No TCP options attached and we never retransmit this. */ - skb = alloc_skb(MAX_TCP_HEADER, priority); + skb = alloc_skb(MAX_TCP_HEADER, sk_allocation(sk, priority)); if (!skb) { NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPABORTFAILED); return; @@ -2242,7 +2243,8 @@ struct sk_buff *tcp_make_synack(struct s struct tcp_md5sig_key *md5; __u8 *md5_hash_location; - skb = sock_wmalloc(sk, MAX_TCP_HEADER + 15, 1, GFP_ATOMIC); + skb = sock_wmalloc(sk, MAX_TCP_HEADER + 15, 1, + sk_allocation(sk, GFP_ATOMIC)); if (skb == NULL) return NULL; @@ -2482,7 +2484,7 @@ void tcp_send_ack(struct sock *sk) * tcp_transmit_skb() will set the ownership to this * sock. */ - buff = alloc_skb(MAX_TCP_HEADER, GFP_ATOMIC); + buff = alloc_skb(MAX_TCP_HEADER, sk_allocation(sk, GFP_ATOMIC)); if (buff == NULL) { inet_csk_schedule_ack(sk); inet_csk(sk)->icsk_ack.ato = TCP_ATO_MIN; @@ -2517,7 +2519,7 @@ static int tcp_xmit_probe_skb(struct soc struct sk_buff *skb; /* We don't queue it, tcp_transmit_skb() sets ownership. */ - skb = alloc_skb(MAX_TCP_HEADER, GFP_ATOMIC); + skb = alloc_skb(MAX_TCP_HEADER, sk_allocation(sk, GFP_ATOMIC)); if (skb == NULL) return -1; Index: linux-2.6/include/net/sock.h =================================================================== --- linux-2.6.orig/include/net/sock.h +++ linux-2.6/include/net/sock.h @@ -435,6 +435,11 @@ static inline int sock_flag(struct sock return test_bit(flag, &sk->sk_flags); } +static inline gfp_t sk_allocation(struct sock *sk, gfp_t gfp_mask) +{ + return gfp_mask; +} + static inline void sk_acceptq_removed(struct sock *sk) { sk->sk_ack_backlog--; Index: linux-2.6/net/ipv6/tcp_ipv6.c =================================================================== --- linux-2.6.orig/net/ipv6/tcp_ipv6.c +++ linux-2.6/net/ipv6/tcp_ipv6.c @@ -582,7 +582,8 @@ static int tcp_v6_md5_do_add(struct sock } else { /* reallocate new list if current one is full. */ if (!tp->md5sig_info) { - tp->md5sig_info = kzalloc(sizeof(*tp->md5sig_info), GFP_ATOMIC); + tp->md5sig_info = kzalloc(sizeof(*tp->md5sig_info), + sk_allocation(sk, GFP_ATOMIC)); if (!tp->md5sig_info) { kfree(newkey); return -ENOMEM; @@ -595,7 +596,8 @@ static int tcp_v6_md5_do_add(struct sock } if (tp->md5sig_info->alloced6 == tp->md5sig_info->entries6) { keys = kmalloc((sizeof (tp->md5sig_info->keys6[0]) * - (tp->md5sig_info->entries6 + 1)), GFP_ATOMIC); + (tp->md5sig_info->entries6 + 1)), + sk_allocation(sk, GFP_ATOMIC)); if (!keys) { tcp_free_md5sig_pool(); @@ -719,7 +721,8 @@ static int tcp_v6_parse_md5_keys (struct struct tcp_sock *tp = tcp_sk(sk); struct tcp_md5sig_info *p; - p = kzalloc(sizeof(struct tcp_md5sig_info), GFP_KERNEL); + p = kzalloc(sizeof(struct tcp_md5sig_info), + sk_allocation(sk, GFP_KERNEL)); if (!p) return -ENOMEM; @@ -952,6 +955,7 @@ static void tcp_v6_send_reset(struct soc #ifdef CONFIG_TCP_MD5SIG struct tcp_md5sig_key *key; #endif + gfp_t gfp_mask = GFP_ATOMIC; if (th->rst) return; @@ -969,13 +973,16 @@ static void tcp_v6_send_reset(struct soc tot_len += TCPOLEN_MD5SIG_ALIGNED; #endif + if (sk) + gfp_mask = sk_allocation(skb->sk, gfp_mask); + /* * We need to grab some memory, and put together an RST, * and then put it into the queue to be sent. */ buff = alloc_skb(MAX_HEADER + sizeof(struct ipv6hdr) + tot_len, - GFP_ATOMIC); + sk_allocation(sk, GFP_ATOMIC)); if (buff == NULL) return; @@ -1063,7 +1070,7 @@ static void tcp_v6_send_ack(struct sk_bu #endif buff = alloc_skb(MAX_HEADER + sizeof(struct ipv6hdr) + tot_len, - GFP_ATOMIC); + sk_allocation(ctl_sk, GFP_ATOMIC)); if (buff == NULL) return; Index: linux-2.6/net/ipv4/tcp.c =================================================================== --- linux-2.6.orig/net/ipv4/tcp.c +++ linux-2.6/net/ipv4/tcp.c @@ -635,7 +635,8 @@ struct sk_buff *sk_stream_alloc_skb(stru /* The TCP header must be at least 32-bit aligned. */ size = ALIGN(size, 4); - skb = alloc_skb_fclone(size + sk->sk_prot->max_header, gfp); + skb = alloc_skb_fclone(size + sk->sk_prot->max_header, + sk_allocation(sk, gfp)); if (skb) { if (sk_wmem_schedule(sk, skb->truesize)) { /*
Introduce sk_allocation(), this function allows to inject sock specific flags to each sock related allocation. Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> --- include/net/sock.h | 5 +++++ net/ipv4/tcp.c | 3 ++- net/ipv4/tcp_output.c | 12 +++++++----- net/ipv6/tcp_ipv6.c | 17 ++++++++++++----- 4 files changed, 26 insertions(+), 11 deletions(-)