| Message ID | 20240623081248.170613-1-sagi@grimberg.me (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: allow skb_datagram_iter to be called from any context | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On 23/06/2024 11:12, Sagi Grimberg wrote: > We only use the mapping in a single context, so kmap_local is sufficient > and cheaper. Make sure to use skb_frag_foreach_page as skb frags may > contain highmem compound pages and we need to map page by page. > > Signed-off-by: Sagi Grimberg <sagi@grimberg.me> > --- > Changes from v2: Changes from v1 actually... > - Fix usercopy BUG() due to copy from highmem pages across page boundary > by using skb_frag_foreach_page > > net/core/datagram.c | 19 +++++++++++++------ > 1 file changed, 13 insertions(+), 6 deletions(-) > > diff --git a/net/core/datagram.c b/net/core/datagram.c > index a8b625abe242..cb72923acc21 100644 > --- a/net/core/datagram.c > +++ b/net/core/datagram.c > @@ -435,15 +435,22 @@ static int __skb_datagram_iter(const struct sk_buff *skb, int offset, > > end = start + skb_frag_size(frag); > if ((copy = end - offset) > 0) { > - struct page *page = skb_frag_page(frag); > - u8 *vaddr = kmap(page); > + u32 p_off, p_len, copied; > + struct page *p; > + u8 *vaddr; > > if (copy > len) > copy = len; > - n = INDIRECT_CALL_1(cb, simple_copy_to_iter, > - vaddr + skb_frag_off(frag) + offset - start, > - copy, data, to); > - kunmap(page); > + > + 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, > + vaddr + p_off, p_len, data, to); > + kunmap_local(vaddr); > + } > + > offset += n; > if (n != copy) > goto short_copy;
On Sun, 2024-06-23 at 11:12 +0300, Sagi Grimberg wrote: > We only use the mapping in a single context, so kmap_local is sufficient > and cheaper. Make sure to use skb_frag_foreach_page as skb frags may > contain highmem compound pages and we need to map page by page. > > Signed-off-by: Sagi Grimberg <sagi@grimberg.me> V1 is already applied to net-next, you need to either send a revert first or share an incremental patch (that would be a fix, and will need a fixes tag). On next revision, please include the target tree in the subj prefix. Thanks, Paolo
On Tue, 25 Jun 2024 15:27:41 +0200 Paolo Abeni wrote: > On Sun, 2024-06-23 at 11:12 +0300, Sagi Grimberg wrote: > > We only use the mapping in a single context, so kmap_local is sufficient > > and cheaper. Make sure to use skb_frag_foreach_page as skb frags may > > contain highmem compound pages and we need to map page by page. > > > > Signed-off-by: Sagi Grimberg <sagi@grimberg.me> > > V1 is already applied to net-next, you need to either send a revert > first or share an incremental patch (that would be a fix, and will need > a fixes tag). > > On next revision, please include the target tree in the subj prefix. I think the bug exists in net (it just requires an arch with HIGHMEM to be hit). So send the fix based on net/master and we'll deal with the net-next conflict? Or you can send a revert for net-next at the same time, but I think the fix should target net.
On Tue, Jun 25, 2024 at 4:10 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 25 Jun 2024 15:27:41 +0200 Paolo Abeni wrote: > > On Sun, 2024-06-23 at 11:12 +0300, Sagi Grimberg wrote: > > > We only use the mapping in a single context, so kmap_local is sufficient > > > and cheaper. Make sure to use skb_frag_foreach_page as skb frags may > > > contain highmem compound pages and we need to map page by page. > > > > > > Signed-off-by: Sagi Grimberg <sagi@grimberg.me> > > > > V1 is already applied to net-next, you need to either send a revert > > first or share an incremental patch (that would be a fix, and will need > > a fixes tag). > > > > On next revision, please include the target tree in the subj prefix. > > I think the bug exists in net (it just requires an arch with HIGHMEM > to be hit). So send the fix based on net/master and we'll deal with > the net-next conflict? Or you can send a revert for net-next at the > same time, but I think the fix should target net. I have not seen a merged patch yet. In any case, some credits would be nice. Reported-by: kernel test robot <oliver.sang@intel.com> Closes: https://lore.kernel.org/oe-lkp/202406161539.b5ff7b20-oliver.sang@intel.com
On Tue, Jun 25, 2024 at 07:10:28AM -0700, Jakub Kicinski wrote: > On Tue, 25 Jun 2024 15:27:41 +0200 Paolo Abeni wrote: > > On Sun, 2024-06-23 at 11:12 +0300, Sagi Grimberg wrote: > > > We only use the mapping in a single context, so kmap_local is sufficient > > > and cheaper. Make sure to use skb_frag_foreach_page as skb frags may > > > contain highmem compound pages and we need to map page by page. > > > > > > Signed-off-by: Sagi Grimberg <sagi@grimberg.me> > > > > V1 is already applied to net-next, you need to either send a revert > > first or share an incremental patch (that would be a fix, and will need > > a fixes tag). > > > > On next revision, please include the target tree in the subj prefix. > > I think the bug exists in net (it just requires an arch with HIGHMEM > to be hit). So send the fix based on net/master and we'll deal with > the net-next conflict? Or you can send a revert for net-next at the > same time, but I think the fix should target net. It does not require an arch with highmem. I was quite clear about this in my earlier email.
diff --git a/net/core/datagram.c b/net/core/datagram.c index a8b625abe242..cb72923acc21 100644 --- a/net/core/datagram.c +++ b/net/core/datagram.c @@ -435,15 +435,22 @@ static int __skb_datagram_iter(const struct sk_buff *skb, int offset, end = start + skb_frag_size(frag); if ((copy = end - offset) > 0) { - struct page *page = skb_frag_page(frag); - u8 *vaddr = kmap(page); + u32 p_off, p_len, copied; + struct page *p; + u8 *vaddr; if (copy > len) copy = len; - n = INDIRECT_CALL_1(cb, simple_copy_to_iter, - vaddr + skb_frag_off(frag) + offset - start, - copy, data, to); - kunmap(page); + + 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, + vaddr + p_off, p_len, data, to); + kunmap_local(vaddr); + } + offset += n; if (n != copy) goto short_copy;
We only use the mapping in a single context, so kmap_local is sufficient and cheaper. Make sure to use skb_frag_foreach_page as skb frags may contain highmem compound pages and we need to map page by page. Signed-off-by: Sagi Grimberg <sagi@grimberg.me> --- Changes from v2: - Fix usercopy BUG() due to copy from highmem pages across page boundary by using skb_frag_foreach_page net/core/datagram.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)