Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 31 Mar 2022 17:48:32 +0200
From:   Ivan Vecera <ivecera@redhat.com>
To:     Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Cc:     netdev@vger.kernel.org,
        "moderated list:INTEL ETHERNET DRIVERS" 
        <intel-wired-lan@lists.osuosl.org>, mschmidt@redhat.com,
        Brett Creeley <brett.creeley@intel.com>,
        open list <linux-kernel@vger.kernel.org>, poros@redhat.com,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "David S. Miller" <davem@davemloft.net>
Subject: Re: [Intel-wired-lan] [PATCH net] ice: Fix incorrect locking in
 ice_vc_process_vf_msg()
Message-ID: <20220331174832.68e17c4a@ceranb>
In-Reply-To: <YkWpNVXYEBo/u3dm@boxer>
References: <20220331105005.2580771-1-ivecera@redhat.com>
        <YkWpNVXYEBo/u3dm@boxer>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, 31 Mar 2022 15:14:29 +0200
Maciej Fijalkowski <maciej.fijalkowski@intel.com> wrote:

> On Thu, Mar 31, 2022 at 12:50:04PM +0200, Ivan Vecera wrote:
> > Usage of mutex_trylock() in ice_vc_process_vf_msg() is incorrect
> > because message sent from VF is ignored and never processed.
> >=20
> > Use mutex_lock() instead to fix the issue. It is safe because this =20
>=20
> We need to know what is *the* issue in the first place.
> Could you please provide more context what is being fixed to the readers
> that don't have an access to bugzilla?
>=20
> Specifically, what is the case that ignoring a particular message when
> mutex is already held is a broken behavior?

Reproducer:

<code>
#!/bin/sh

set -xe

PF=3D"ens7f0"
VF=3D"${PF}v0"

echo 1 > /sys/class/net/${PF}/device/sriov_numvfs
sleep 2

ip link set ${VF} up
ip addr add 172.30.29.11/24 dev ${VF}

while true; do

# Set VF to be trusted
ip link set ${PF} vf 0 trust on

# Ping server again
ping -c5 172.30.29.2 || {
        echo Ping failed
        ip link show dev ${VF} # <- No carrier here
        break
}

ip link set ${PF} vf 0 trust off
sleep 1

done

echo 0 > /sys/class/net/${PF}/device/sriov_numvfs
</code>

<sample>
[root@wsfd-advnetlab150 ~]# uname -r
5.17.0+ # Current net.git HEAD
[root@wsfd-advnetlab150 ~]# ./repro_simple.sh=20
+ PF=3Dens7f0
+ VF=3Dens7f0v0
+ echo 1
+ sleep 2
+ ip link set ens7f0v0 up
+ ip addr add 172.30.29.11/24 dev ens7f0v0
+ true
+ ip link set ens7f0 vf 0 trust on
+ ping -c5 172.30.29.2
PING 172.30.29.2 (172.30.29.2) 56(84) bytes of data.
64 bytes from 172.30.29.2: icmp_seq=3D2 ttl=3D64 time=3D0.820 ms
64 bytes from 172.30.29.2: icmp_seq=3D3 ttl=3D64 time=3D0.142 ms
64 bytes from 172.30.29.2: icmp_seq=3D4 ttl=3D64 time=3D0.128 ms
64 bytes from 172.30.29.2: icmp_seq=3D5 ttl=3D64 time=3D0.129 ms

--- 172.30.29.2 ping statistics ---
5 packets transmitted, 4 received, 20% packet loss, time 4110ms
rtt min/avg/max/mdev =3D 0.128/0.304/0.820/0.298 ms
+ ip link set ens7f0 vf 0 trust off
+ sleep 1
+ true
+ ip link set ens7f0 vf 0 trust on
+ ping -c5 172.30.29.2
PING 172.30.29.2 (172.30.29.2) 56(84) bytes of data.
=46rom 172.30.29.11 icmp_seq=3D1 Destination Host Unreachable
=46rom 172.30.29.11 icmp_seq=3D2 Destination Host Unreachable
=46rom 172.30.29.11 icmp_seq=3D3 Destination Host Unreachable

--- 172.30.29.2 ping statistics ---
5 packets transmitted, 0 received, +3 errors, 100% packet loss, time 4125ms
pipe 3
+ echo Ping failed
Ping failed
+ ip link show dev ens7f0v0
20: ens7f0v0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc mq state D=
OWN mode DEFAULT group default qlen 1000
    link/ether de:69:e3:a5:68:b6 brd ff:ff:ff:ff:ff:ff
    altname enp202s0f0v0
+ break
+ echo 0

[root@wsfd-advnetlab150 ~]# dmesg | tail -8
[  220.265891] iavf 0000:ca:01.0: Reset indication received from the PF
[  220.272250] iavf 0000:ca:01.0: Scheduling reset task
[  220.277217] iavf 0000:ca:01.0: Hardware reset detected
[  220.292854] ice 0000:ca:00.0: VF 0 is now trusted
[  220.295027] ice 0000:ca:00.0: VF 0 is being configured in another contex=
t that will trigger a VFR, so there is no need to handle this message
[  234.445819] iavf 0000:ca:01.0: PF returned error -64 (IAVF_NOT_SUPPORTED=
) to our request 9
[  234.466827] iavf 0000:ca:01.0: Failed to delete MAC filter, error IAVF_N=
OT_SUPPORTED
[  234.474574] iavf 0000:ca:01.0: Remove device
</sample>

User set VF to be trusted so .ndo_set_vf_trust (ice_set_vf_trust) is called.
Function ice_set_vf_trust() takes vf->cfg_lock and calls ice_vc_reset_vf() =
that
sends message to iavf that initiates reset task. During this reset task iav=
f sends
config messages to ice. These messages are handled in ice_service_task() co=
ntext
via ice_clean_adminq_subtask() -> __ice_clean_ctrlq() -> ice_vc_process_vf_=
msg().

Function ice_vc_process_vf_msg() tries to take vf->cfg_lock but this can be=
 locked
from ice_set_vf_trust() yet (as in sample above). The lock attempt failed s=
o the function
returns, message is not processed.

Thanks,
Ivan