ACK/cmnt: [SRU][Xenial][PATCH 0/7] Fixes for LP#1715812

[Stefan Bader]

On 08.09.2017 09:01, Daniel Axtens wrote:
> (This is the Xenial patchset. Some patches required minor backporting.)
> 
> [SRU Justification]
> 
> [Impact]
> A host can lose access to another host whose MAC address changes if
> they have active connections to other hosts that share a route. The
> ARP cache does not time out as expected - instead the old MAC address
> is continuously reconfirmed.
> 
> [Fix]
> Apply series [1], which changes the algorithm for neighbour confirmation.
> That is, from upstream:
> 51ce8bd4d17a net: pending_confirm is not used anymore
> 0dec879f636f net: use dst_confirm_neigh for UDP, RAW, ICMP, L2TP
> 63fca65d0863 net: add confirm_neigh method to dst_ops
> c3a2e8370534 tcp: replace dst_confirm with sk_dst_confirm
> c86a773c7802 sctp: add dst_pending_confirm flag
> 4ff0620354f2 net: add dst_pending_confirm flag to skbuff
> 9b8805a32559 sock: add sk_dst_pending_confirm flag
> 
> [Test case]
> Create 3 real or virtual systems, all hooked up to a switch.
> One system needs an active-backup bond with fail_over_mac=1 num_grat_arp=0.
> 
> Put all the systems in the same subnet, e.g. 192.168.200.0/24
> 
> Call the system with the bond A, and the other two systems B and C.
> 
> On B, run in 3 shells:
>  - netperf -t TCP_RR to C
>  - ping -f A
>  - watch 'ip -s neigh show 192.168.200.0/24'
> 
> On A, cause the bond to fail over.
> 
> Observe that:
> 
>  - without the patches, B intermittently fails to notice the change in
>    A's MAC address. This presents as the ping failing and not
>    recovering, and the arp table showing the old mac address never
>    timing out and never being replace with a new mac address.
> 
>  - with the patches, the arp cache times out and B sends another mac
>    probe and detects A's new address.
> 
> It helps to use taskset to put ping and netperf on the same CPU, or use single-CPU vms.
> 
> See [2] for more details.
> 
> [References]
> [2] Original report: https://www.mail-archive.com/netdev@vger.kernel.org/msg138762.html
> [1]: https://www.spinics.net/lists/linux-rdma/msg45907.html
> 
> Julian Anastasov (7):
>   sock: add sk_dst_pending_confirm flag
>   net: add dst_pending_confirm flag to skbuff
>   sctp: add dst_pending_confirm flag
>   tcp: replace dst_confirm with sk_dst_confirm
>   net: add confirm_neigh method to dst_ops
>   net: use dst_confirm_neigh for UDP, RAW, ICMP, L2TP
>   net: pending_confirm is not used anymore
> 
>  drivers/net/vrf.c          |  5 ++++-
>  include/linux/skbuff.h     | 12 ++++++++++++
>  include/net/arp.h          | 16 ++++++++++++++++
>  include/net/dst.h          | 21 +++++++++------------
>  include/net/dst_ops.h      |  2 ++
>  include/net/ndisc.h        | 17 +++++++++++++++++
>  include/net/sctp/sctp.h    |  6 ++----
>  include/net/sctp/structs.h |  4 ++++
>  include/net/sock.h         | 25 +++++++++++++++++++++++++
>  net/core/dst.c             |  1 -
>  net/core/sock.c            |  2 ++
>  net/ipv4/ip_output.c       | 11 ++++++++++-
>  net/ipv4/ping.c            |  3 ++-
>  net/ipv4/raw.c             |  6 +++++-
>  net/ipv4/route.c           | 19 +++++++++++++++++++
>  net/ipv4/tcp_input.c       | 12 +++---------
>  net/ipv4/tcp_metrics.c     |  7 ++-----
>  net/ipv4/tcp_output.c      |  2 ++
>  net/ipv4/udp.c             |  3 ++-
>  net/ipv6/ip6_output.c      |  7 +++++++
>  net/ipv6/raw.c             |  6 +++++-
>  net/ipv6/route.c           | 43 ++++++++++++++++++++++++++++++-------------
>  net/ipv6/udp.c             |  3 ++-
>  net/l2tp/l2tp_ip6.c        |  3 ++-
>  net/sctp/associola.c       |  3 +--
>  net/sctp/output.c          | 10 +++++++++-
>  net/sctp/outqueue.c        |  2 +-
>  net/sctp/sm_make_chunk.c   |  6 ++----
>  net/sctp/sm_sideeffect.c   |  2 +-
>  net/sctp/socket.c          |  4 ++--
>  net/sctp/transport.c       | 16 +++++++++++++++-
>  net/xfrm/xfrm_policy.c     | 19 +++++++++++++++++++
>  32 files changed, 234 insertions(+), 64 deletions(-)
> 
Acked-by: Stefan Bader <stefan.bader at canonical.com>

Trusting Colin did test the backports individually (because the related bug
report does not explicitly state whether the test case has ben run with test
kernels for Zesty and/or Xenial).

One thing I forgot to mention in the Zesty ACK: I also only found that one fixup
which Colin mentioned. The only noticeable difference would be that the stack
usage is minimally lower due to the dropped local variable. I would not thing
this warrants its inclusion but would leave it to Daniel or Jay to decide.

-Stefan

-------------- 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/20170915/622dd44e/attachment.sig>