| Message ID | 20240626070037.758538-1-sagi@grimberg.me (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3] net: allow skb_datagram_iter to be called from any context | expand |
On Wed, Jun 26, 2024 at 9:00 AM Sagi Grimberg <sagi@grimberg.me> 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. > > Reported-by: kernel test robot <oliver.sang@intel.com> > Closes: https://lore.kernel.org/oe-lkp/202406161539.b5ff7b20-oliver.sang@intel.com > Signed-off-by: Sagi Grimberg <sagi@grimberg.me> Thanks for working on this ! A patch targeting net tree would need a Fixes: tag, so that stable teams do not have to do archeological digging to find which trees need this fix. If the bug is too old, then maybe this patch should use kmap() instead of kmap_local_page() ? Then in net-next, (after this fix is merged), perform the conversion to kmap_local_page() Fact that the bug never showed up is a bit strange, are 32bit systems still used today ? (apart from bots)... Do we have a reproducer to test this? > --- > Changes from v2: > - added a target tree in subject prefix > - added reported credits and closes annotation > > Changes from v1: > - 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 e614cfd8e14a..e9ba4c7b449d 100644 > --- a/net/core/datagram.c > +++ b/net/core/datagram.c > @@ -416,15 +416,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; > -- > 2.43.0 >
On 26/06/2024 10:40, Eric Dumazet wrote: > On Wed, Jun 26, 2024 at 9:00 AM Sagi Grimberg <sagi@grimberg.me> 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. >> >> Reported-by: kernel test robot <oliver.sang@intel.com> >> Closes: https://lore.kernel.org/oe-lkp/202406161539.b5ff7b20-oliver.sang@intel.com >> Signed-off-by: Sagi Grimberg <sagi@grimberg.me> > Thanks for working on this ! > > A patch targeting net tree would need a Fixes: tag, so that stable > teams do not have > to do archeological digging to find which trees need this fix. The BUG complaint was exposed by the reverted patch in net-next. TBH, its hard to tell when this actually was introduced, could skb_frags always have contained high-order pages? or was this introduced with the introduction of skb_copy_datagram_iter? or did it originate in the base implementation it was copied from skb_copy_datagram_const_iovec? Its hard for me to tell, and I don't have enough knowledge of the stack. could use some help here. > > If the bug is too old, then maybe this patch should use kmap() instead > of kmap_local_page() ? I honestly don't know. > > Then in net-next, (after this fix is merged), perform the conversion > to kmap_local_page() > > Fact that the bug never showed up is a bit strange, are 32bit systems > still used today ? (apart from bots)... Not sure. > > Do we have a reproducer to test this? Aside from the robot not that I know of...
On Wed, Jun 26, 2024 at 10:23 AM Sagi Grimberg <sagi@grimberg.me> wrote: > > > > On 26/06/2024 10:40, Eric Dumazet wrote: > > On Wed, Jun 26, 2024 at 9:00 AM Sagi Grimberg <sagi@grimberg.me> 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. > >> > >> Reported-by: kernel test robot <oliver.sang@intel.com> > >> Closes: https://lore.kernel.org/oe-lkp/202406161539.b5ff7b20-oliver.sang@intel.com > >> Signed-off-by: Sagi Grimberg <sagi@grimberg.me> > > Thanks for working on this ! > > > > A patch targeting net tree would need a Fixes: tag, so that stable > > teams do not have > > to do archeological digging to find which trees need this fix. > > The BUG complaint was exposed by the reverted patch in net-next. > > TBH, its hard to tell when this actually was introduced, could skb_frags > always > have contained high-order pages? or was this introduced with the > introduction > of skb_copy_datagram_iter? or did it originate in the base implementation it > was copied from skb_copy_datagram_const_iovec? OK, I will therefore suggest something like this (even if the older bug might be exposed by a more recent patch), to cover all kernels after 5.0 This was the commit adding __skb_datagram_iter(), not the bug. Fixes: 950fcaecd5cc ("datagram: consolidate datagram copy to iter helpers") I suspect high-order highmem pages were not used in older kernels anyway.
On Wed, Jun 26, 2024 at 10:43:38AM +0200, Eric Dumazet wrote: > On Wed, Jun 26, 2024 at 10:23 AM Sagi Grimberg <sagi@grimberg.me> wrote: > > > > > > > > On 26/06/2024 10:40, Eric Dumazet wrote: > > > On Wed, Jun 26, 2024 at 9:00 AM Sagi Grimberg <sagi@grimberg.me> 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. > > >> > > >> Reported-by: kernel test robot <oliver.sang@intel.com> > > >> Closes: https://lore.kernel.org/oe-lkp/202406161539.b5ff7b20-oliver.sang@intel.com > > >> Signed-off-by: Sagi Grimberg <sagi@grimberg.me> > > > Thanks for working on this ! > > > > > > A patch targeting net tree would need a Fixes: tag, so that stable > > > teams do not have > > > to do archeological digging to find which trees need this fix. > > > > The BUG complaint was exposed by the reverted patch in net-next. > > > > TBH, its hard to tell when this actually was introduced, could skb_frags > > always > > have contained high-order pages? or was this introduced with the > > introduction > > of skb_copy_datagram_iter? or did it originate in the base implementation it > > was copied from skb_copy_datagram_const_iovec? > > OK, I will therefore suggest something like this (even if the older > bug might be exposed > by a more recent patch), to cover all kernels after 5.0 > > This was the commit adding __skb_datagram_iter(), not the bug. > > Fixes: 950fcaecd5cc ("datagram: consolidate datagram copy to iter helpers") > > I suspect high-order highmem pages were not used in older kernels anyway. This explanation is slightly garbled. High-order lowmem pages _also_ get caught by this check. See my earlier email explaining this.
diff --git a/net/core/datagram.c b/net/core/datagram.c index e614cfd8e14a..e9ba4c7b449d 100644 --- a/net/core/datagram.c +++ b/net/core/datagram.c @@ -416,15 +416,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. Reported-by: kernel test robot <oliver.sang@intel.com> Closes: https://lore.kernel.org/oe-lkp/202406161539.b5ff7b20-oliver.sang@intel.com Signed-off-by: Sagi Grimberg <sagi@grimberg.me> --- Changes from v2: - added a target tree in subject prefix - added reported credits and closes annotation Changes from v1: - 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(-)