(not NACK): [PATCH 0/3][Artful] fix skb leak in vhost_net / tun / tap (30% perf regression)

[Khaled Elmously]

On 2018-02-01 09:29:46 , Stefan Bader wrote:
> On 31.01.2018 19:44, Khaled Elmously wrote:
> > On 2017-12-19 10:09:53 , Fabian Grünbichler wrote:
> >> BugLink: https://bugs.launchpad.net/1738975
> >>
> >> == SRU Justification ==
> >>
> >> Impact: Up to 30% performance regression for traffic from hypervisor host to VM, caused by a memory leak
> >>
> >> Fix: Cherry-picks from upstream stable tree to fix the memory leak
> >>
> >> Regression Potential: Merged in 4.15 and 4.14.7, tested and verified by multiple people.
> >>
> >> See https://lkml.kernel.org/r/<4c7e2924-b10f-0e97-c388-c8809ecfdeeb@linux.vnet.ibm.com> for the original report and extensive discussion
> >> See https://lkml.kernel.org/r/<1512123038-15773-1-git-send-email-wexu@redhat.com> for the patch series in question
> >>
> >> Wei Xu (3):
> >>   vhost: fix skb leak in handle_rx()
> >>   tap: free skb if flags error
> >>   tun: free skb in early errors
> >>
> >>  drivers/net/tap.c   | 14 ++++++++++----
> >>  drivers/net/tun.c   | 24 ++++++++++++++++++------
> >>  drivers/vhost/net.c | 20 ++++++++++----------
> >>  3 files changed, 38 insertions(+), 20 deletions(-)
> >>
> > 
> > As Stefan pointed out, the BugLink should point to a valid bug report.
> 
> Though Fabian replied since then to the list and hinted that it is just missing
> s .../bug/... element in the URL. I had not realized this when I went through
> the backlog.
> 

I see it now as well, thanks.

> > 
> > Also, the 'cherry picked from commit ...' line should refer to the mainline tree, however you seem to be cherry-picking from linux-stable. Please update the cherry-pick line to say 'cherry picked from commit ... linux-stable' or, preferably, just cherry-pick from mainline directly.
> 
> There are cases when cherry-picking from a stable tree is preferable: that is if
> the patch had to be adapted on its way back. If the same patch was already
> backported for a stable tree close (in this case 4.14.y) and can be just
> cherry-picked from there it is less work and less prone to errors. I would add a
> tree reference to the reference line though. Like
> 
> (cherry picked from commit 0536add671963d9875dc42f734d1608bef480dc7 linux-2.14.y)

Thanks for the explanation. It seems in this case the linux-stable commit was simply cherry picked from mainline so there's no difference. But agreed, no point in resending.

So this patch looks good to me now, I intend to ACK it. If you are also happy with it, please ACK it and I will apply it when I get to it in my inbox.

Thanks.


> 
> And we could do that when applying things, as long as the info is somewhere in
> the submission. No need to send again... well except if someone keeps sending
> things the wrong way after they have been told, or the reviewer had not enough
> coffee or is generally grumpy... ;)
> 
> -Stefan
> 
> -Stefan
> > 
> > Otherwise, the actual patches look fine.
> > 
> > For the above reasons, NACK.
> > 
> 
> 

-Khaled