X would not start in my old 32-bit partition (and the "n"-handling looks
just as wrong on 64-bit, but for whatever reason did not show up there):
"n" must be accumulated over all pages before it's added to "offset" and
compared with "copy", immediately after the skb_frag_foreach_page() loop.

Fixes: d2d30a376d9c ("net: allow skb_datagram_iter to be called from any context")
Signed-off-by: Hugh Dickins <hughd@google.com>
---
 net/core/datagram.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/datagram.c b/net/core/datagram.c
index e9ba4c7b449d..ea69d01156e6 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c
@@ -420,6 +420,7 @@ static int __skb_datagram_iter(const struct sk_buff *skb, int offset,
 			struct page *p;
 			u8 *vaddr;
 
+			n = 0;
 			if (copy > len)
 				copy = len;
 
@@ -427,7 +428,7 @@ static int __skb_datagram_iter(const struct sk_buff *skb, int offset,
 					      skb_frag_off(frag) + offset - start,
 					      copy, p, p_off, p_len, copied) {
 				vaddr = kmap_local_page(p);
-				n = INDIRECT_CALL_1(cb, simple_copy_to_iter,
+				n += INDIRECT_CALL_1(cb, simple_copy_to_iter,
 					vaddr + p_off, p_len, data, to);
 				kunmap_local(vaddr);
 			}
-- 
2.35.3

On 08/07/2024 6:00, Hugh Dickins wrote:
> X would not start in my old 32-bit partition (and the "n"-handling looks
> just as wrong on 64-bit, but for whatever reason did not show up there):
> "n" must be accumulated over all pages before it's added to "offset" and
> compared with "copy", immediately after the skb_frag_foreach_page() loop.

That is indeed strange. I see the issue. It didn't happen in my local 
testing either.

>
> Fixes: d2d30a376d9c ("net: allow skb_datagram_iter to be called from any context")
> Signed-off-by: Hugh Dickins <hughd@google.com>
> ---
>   net/core/datagram.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/datagram.c b/net/core/datagram.c
> index e9ba4c7b449d..ea69d01156e6 100644
> --- a/net/core/datagram.c
> +++ b/net/core/datagram.c
> @@ -420,6 +420,7 @@ static int __skb_datagram_iter(const struct sk_buff *skb, int offset,
>   			struct page *p;
>   			u8 *vaddr;
>   
> +			n = 0;

I think its better to reset n right before the skb_frag_foreach_page() 
iteration.

Thanks Hugh for addressing this!

X would not start in my old 32-bit partition (and the "n"-handling looks
just as wrong on 64-bit, but for whatever reason did not show up there):
"n" must be accumulated over all pages before it's added to "offset" and
compared with "copy", immediately after the skb_frag_foreach_page() loop.

Fixes: d2d30a376d9c ("net: allow skb_datagram_iter to be called from any context")
Signed-off-by: Hugh Dickins <hughd@google.com>
---
v2: moved the "n = 0" down, per Sagi: no functional change.

 net/core/datagram.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/datagram.c b/net/core/datagram.c
index e9ba4c7b449d..e72dd78471a6 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c
@@ -423,11 +423,12 @@ static int __skb_datagram_iter(const struct sk_buff *skb, int offset,
 			if (copy > len)
 				copy = len;
 
+			n = 0;
 			skb_frag_foreach_page(frag,
 					      skb_frag_off(frag) + offset - start,
 					      copy, p, p_off, p_len, copied) {
 				vaddr = kmap_local_page(p);
-				n = INDIRECT_CALL_1(cb, simple_copy_to_iter,
+				n += INDIRECT_CALL_1(cb, simple_copy_to_iter,
 					vaddr + p_off, p_len, data, to);
 				kunmap_local(vaddr);
 			}
-- 
2.35.3

On 08/07/2024 17:46, Hugh Dickins wrote:
> X would not start in my old 32-bit partition (and the "n"-handling looks
> just as wrong on 64-bit, but for whatever reason did not show up there):
> "n" must be accumulated over all pages before it's added to "offset" and
> compared with "copy", immediately after the skb_frag_foreach_page() loop.
>
> Fixes: d2d30a376d9c ("net: allow skb_datagram_iter to be called from any context")
> Signed-off-by: Hugh Dickins <hughd@google.com>

Thanks Hugh,

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

X would not start in my old 32-bit partition (and the "n"-handling looks
just as wrong on 64-bit, but for whatever reason did not show up there):
"n" must be accumulated over all pages before it's added to "offset" and
compared with "copy", immediately after the skb_frag_foreach_page() loop.

Fixes: d2d30a376d9c ("net: allow skb_datagram_iter to be called from any context")
Signed-off-by: Hugh Dickins <hughd@google.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
v3: added reviewed-by Sagi, try sending direct to Linus
v2: moved the "n = 0" down, per Sagi: no functional change.

 net/core/datagram.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/datagram.c b/net/core/datagram.c
index e9ba4c7b449d..e72dd78471a6 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c
@@ -423,11 +423,12 @@ static int __skb_datagram_iter(const struct sk_buff *skb, int offset,
 			if (copy > len)
 				copy = len;
 
+			n = 0;
 			skb_frag_foreach_page(frag,
 					      skb_frag_off(frag) + offset - start,
 					      copy, p, p_off, p_len, copied) {
 				vaddr = kmap_local_page(p);
-				n = INDIRECT_CALL_1(cb, simple_copy_to_iter,
+				n += INDIRECT_CALL_1(cb, simple_copy_to_iter,
 					vaddr + p_off, p_len, data, to);
 				kunmap_local(vaddr);
 			}
-- 
2.35.3

On Wed, 2024-07-10 at 08:36 -0700, Hugh Dickins wrote:
> X would not start in my old 32-bit partition (and the "n"-handling looks
> just as wrong on 64-bit, but for whatever reason did not show up there):
> "n" must be accumulated over all pages before it's added to "offset" and
> compared with "copy", immediately after the skb_frag_foreach_page() loop.
> 
> Fixes: d2d30a376d9c ("net: allow skb_datagram_iter to be called from any context")
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> ---
> v3: added reviewed-by Sagi, try sending direct to Linus

V2 is already applied to the 'net' tree and will be included in our
next 'net' PR, coming tomorrow.

It looks like the netdev bot decided it needed an holiday (or was
fooled by the threaded submission for v2), so no notification landed on
the ML.

@Hugh: next time please check the current tree status or patchwork
before submitting a new revision. And please avoid submitting the new
version in reply to a previous one, it makes things difficult for our
CI.

Thanks,

Paolo

On Wed, 10 Jul 2024, Paolo Abeni wrote:
> On Wed, 2024-07-10 at 08:36 -0700, Hugh Dickins wrote:
> > X would not start in my old 32-bit partition (and the "n"-handling looks
> > just as wrong on 64-bit, but for whatever reason did not show up there):
> > "n" must be accumulated over all pages before it's added to "offset" and
> > compared with "copy", immediately after the skb_frag_foreach_page() loop.
> > 
> > Fixes: d2d30a376d9c ("net: allow skb_datagram_iter to be called from any context")
> > Signed-off-by: Hugh Dickins <hughd@google.com>
> > Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> > ---
> > v3: added reviewed-by Sagi, try sending direct to Linus
> 
> V2 is already applied to the 'net' tree and will be included in our
> next 'net' PR, coming tomorrow.
> 
> It looks like the netdev bot decided it needed an holiday (or was
> fooled by the threaded submission for v2), so no notification landed on
> the ML.
> 
> @Hugh: next time please check the current tree status or patchwork
> before submitting a new revision. And please avoid submitting the new
> version in reply to a previous one, it makes things difficult for our
> CI.

Ah, great, thanks. Yes, I'd heard only silence (and was worried because,
although I only saw the effect of this on 32-bit, suspect it could cause
lots of obscure trouble more generally).

Hugh