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

[Stefan Bader]

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.

> 
> 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)

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.
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20180201/46cd6e94/attachment.sig>