On Thu, 2023-05-18 at 10:53 +0100, David Howells wrote:
> Paolo Abeni <pabeni@xxxxxxxxxx> wrote:
>
> > Minor nit: please respect the reverse x-mas tree order (there are a few
> > other occurrences around)
>
> I hadn't come across that. Normally I only apply that to the types so that
> the names aren't all over the place. But whatever.
>
> > > + if (space == 0 &&
> > > + !skb_can_coalesce(skb, skb_shinfo(skb)->nr_frags,
> > > + pages[0], off)) {
> > > + iov_iter_revert(iter, len);
> > > + break;
> > > + }
> >
> > It looks like the above condition/checks duplicate what the later
> > skb_append_pagefrags() will perform below. I guess the above chunk
> > could be removed?
>
> Good point. There used to be an allocation between in the case sendpage_ok()
> failed and we wanted to copy the data. I've removed that for the moment.
>
> > > + ret = -EIO;
> > > + if (!sendpage_ok(page))
> > > + goto out;
> >
> > My (limited) understanding is that the current sendpage code assumes
> > that the caller provides/uses pages suitable for such use. The existing
> > sendpage_ok() check is in place as way to try to catch possible code
> > bug - via the WARN_ONCE().
> >
> > I think the same could be done here?
>
> Yeah.
>
> Okay, I made the attached changes to this patch.
>
> David
> ---
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 56d629ea2f3d..f4a5b51aed22 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -6923,10 +6923,10 @@ static void skb_splice_csum_page(struct sk_buff *skb, struct page *page,
> ssize_t skb_splice_from_iter(struct sk_buff *skb, struct iov_iter *iter,
> ssize_t maxsize, gfp_t gfp)
> {
> + size_t frag_limit = READ_ONCE(sysctl_max_skb_frags);
> struct page *pages[8], **ppages = pages;
> - unsigned int i;
> ssize_t spliced = 0, ret = 0;
> - size_t frag_limit = READ_ONCE(sysctl_max_skb_frags);
> + unsigned int i;
>
> while (iter->count > 0) {
> ssize_t space, nr;
> @@ -6946,20 +6946,13 @@ ssize_t skb_splice_from_iter(struct sk_buff *skb, struct iov_iter *iter,
> break;
> }
>
> - if (space == 0 &&
> - !skb_can_coalesce(skb, skb_shinfo(skb)->nr_frags,
> - pages[0], off)) {
> - iov_iter_revert(iter, len);
> - break;
> - }
> -
> i = 0;
> do {
> struct page *page = pages[i++];
> size_t part = min_t(size_t, PAGE_SIZE - off, len);
>
> ret = -EIO;
> - if (!sendpage_ok(page))
> + if (WARN_ON_ONCE(!sendpage_ok(page)))
FWIS the current TCP code also has a 'IS_ENABLED(CONFIG_DEBUG_VM) &&'
guard, but I guess the plain WARN_ON_ONCE should be ok.
Side node: we need the whole series alltogether, you need to repost
even the unmodified patches.
Thanks!
Paolo
