Re: [PATCH net] tcp: fix page frag corruption on page fault
From: Eric Dumazet <edumazet@google.com>
Date: 2021-11-26 15:35:16
On Fri, Nov 26, 2021 at 7:05 AM Paolo Abeni [off-list ref] wrote:
Hello,
...
Double checking I understood correctly: the problem is that in case of skb_copy_to_page_nocache() error, if the skb is not empty, random data will be introduced into the TCP stream, or something else/more? I obviously did not see that before the submission, nor tests cached it, sorry.
If there is a copy error, the skb is left with an extended fragment (if a merge happened) or a new frag, with random data in it, as the copy operation failed. We are thus going to send garbage on the network on next sendmsg(), which will append more stuff in the frag array.
quoted
Please investigate CIFS and gfpflags_normal_context() tandem to fix this issue instead.Do you mean changing gfpflags_normal_context() definition so that cifs allocation are excluded? Something alike the following should do:
We need to find one flag that can ask gfpflags_normal_context() to return false for this case. Or if too difficult, stop only using sk->sk_allocation to decide between the per-thread frag, or the per socket one. I presume there are few active CIFS sockets on a host. They should use a per socket sk_frag.
quoted hunk ↗ jump to hunk
---diff --git a/include/linux/gfp.h b/include/linux/gfp.h index b976c4177299..f9286aeeded5 100644 --- a/include/linux/gfp.h +++ b/include/linux/gfp.h@@ -379,8 +379,8 @@ static inline bool gfpflags_allow_blocking(const gfp_t gfp_flags) */ static inline bool gfpflags_normal_context(const gfp_t gfp_flags) { - return (gfp_flags & (__GFP_DIRECT_RECLAIM | __GFP_MEMALLOC)) == - __GFP_DIRECT_RECLAIM; + return (gfp_flags & (__GFP_DIRECT_RECLAIM | __GFP_MEMALLOC | __GFP_FS)) == + (__GFP_DIRECT_RECLAIM | __GFP_FS); }--- If so there is a caveat: dlm is currently using gfpflags_normal_context() - apparently to check for non blocking context. If I'll change gfpflags_normal_context() definition I likely will have to replace gfpflags_normal_context() with gfpflags_allow_blocking() in dlm. In that case the relevant patch should touch both the mm and the fs subsystem. In that case I guess I should go via the fs tree first and the via mm? Thanks! Paolo