4.14.0-rc3 is auto-reloading PCIe SR-IOV Virtual Function device drivers when VFs are attached to VMs ...

4.14.0-rc3 is auto-reloading PCIe SR-IOV Virtual Function device drivers when VFs are attached to VMs ...

[Casey Leedom]

Hi there,

  I hope this is the right Linux list for this issue.  One of our QA staff
has noticed a new behavior starting about 4.14.0-rc3.  We instantiate PCIe
SR-IOV Virtual Functions and the corresponding driver (cxgb4vf in this case=
)
is automatically loaded.  That's fine and has been the case for some time.
But, we remove the driver module (rmmod cxgb4vf) and then attach one of the
VFs to a KVM Virtual Machine and the cxgb4vf driver module gets reloaded in
the KVM Hypervisor.  This is new behavior.  I see that there was a pretty
big change done by Luis R. Rodriguez in kernel.org commit revision 2355869
but we haven't yet isolated the new behavior to that commit.  That'll be ou=
r
next test but I wanted to get this out there for comment.

Casey=

Re: 4.14.0-rc3 is auto-reloading PCIe SR-IOV Virtual Function device drivers when VFs are attached to VMs ...

[Bjorn Helgaas]

[+cc Alex, Bryant, Steven, Juan]

On Tue, Oct 17, 2017 at 1:36 PM, Casey Leedom <leedom@chelsio.com> wrote:
>
> Hi there,
>
>   I hope this is the right Linux list for this issue.  One of our QA staff
> has noticed a new behavior starting about 4.14.0-rc3.  We instantiate PCIe
> SR-IOV Virtual Functions and the corresponding driver (cxgb4vf in this case)
> is automatically loaded.  That's fine and has been the case for some time.
> But, we remove the driver module (rmmod cxgb4vf) and then attach one of the
> VFs to a KVM Virtual Machine and the cxgb4vf driver module gets reloaded in
> the KVM Hypervisor.  This is new behavior.  I see that there was a pretty
> big change done by Luis R. Rodriguez in kernel.org commit revision 2355869
> but we haven't yet isolated the new behavior to that commit.  That'll be our
> next test but I wanted to get this out there for comment.

This is a good list.  I don't know what commit revision 2355869 refers
to.  It would be helpful if you could cite the git SHA1 for the
commit.

We're also having a conversation about the question of binding drivers
to VFs on powerpc here:
https://lkml.kernel.org/r/20170922140608.47665-3-bryantly@linux.vnet.ibm.com

It's not exactly the same issue, but you might have useful input there.

Re: 4.14.0-rc3 is auto-reloading PCIe SR-IOV Virtual Function device drivers when VFs are attached to VMs ...

[Alex Williamson]

On Tue, 17 Oct 2017 18:36:21 +0000
Casey Leedom <leedom@chelsio.com> wrote:

> Hi there,
> 
>   I hope this is the right Linux list for this issue.  One of our QA staff
> has noticed a new behavior starting about 4.14.0-rc3.  We instantiate PCIe
> SR-IOV Virtual Functions and the corresponding driver (cxgb4vf in this case)
> is automatically loaded.  That's fine and has been the case for some time.
> But, we remove the driver module (rmmod cxgb4vf) and then attach one of the
> VFs to a KVM Virtual Machine and the cxgb4vf driver module gets reloaded in
> the KVM Hypervisor.  This is new behavior.  I see that there was a pretty
> big change done by Luis R. Rodriguez in kernel.org commit revision 2355869
> but we haven't yet isolated the new behavior to that commit.  That'll be our
> next test but I wanted to get this out there for comment.

I'm not sure I understand the problem, generally when doing device
assignment you're using vfio-pci, which I'll assume is the case since
we're talking about an upstream kernel and Legacy KVM device assignment
no longer exists upstream.  That means the VF being assigned needs to
be bound to the vfio-pci driver, which libvirt might do for you if the
hostdev XML is specified with managed='yes'.  At what point is cxgb4vf
being re-loaded?  Is it correlated with the VM starting or VF being
hot-added to a VM or the reverse, VM shutdown or VF removed?  Is the
cxgb4vf driver replacing vfio-pci for the device being assigned?
libvirt might do a drivers_probe on remove, but that behavior shouldn't
have changed.  Being a VF, it needs to support FLR, so that would be
the means vfio would use to reset the device.  I wouldn't expect an FLR
to trigger any sort of device remove/re-add that might cause udev to
reload the driver.  Presumably adding a modprobe.d blacklist entry for
cxgb4vf would resolve it, if so, maybe look at udev events.  If there
are multiple PFs in the host, any creation of new VFs matching the
cxgb4vf driver could trigger udev to re-load the driver.  Thanks,

Alex

Re: 4.14.0-rc3 is auto-reloading PCIe SR-IOV Virtual Function device drivers when VFs are attached to VMs ...

[Casey Leedom]

  I'm not sure what the exact mechanism is that our QA person Komali used,
she'll need to respond to that.  The short summary of her report is:

------------
1>Loaded cxgb4 on Host.

2>Instantiated Max number of VF's (64) using below command, Observe cxgb4vf
automatically loaded, able to see all 64 VF's on the Host.

echo 16 > /sys/class/net/enp3s0f4/device/driver/0000\:03\:00.0/sriov_numvfs
echo 16 > /sys/class/net/enp3s0f4/device/driver/0000\:03\:00.1/sriov_numvfs
echo 16 > /sys/class/net/enp3s0f4/device/driver/0000\:03\:00.2/sriov_numvfs
echo 16 > /sys/class/net/enp3s0f4/device/driver/0000\:03\:00.3/sriov_numvfs

[root@t5nic ~]# lsmod | grep -i cxgb4
cxgb4vf                73728  0
iw_cxgb4              204800  0
ib_core               212992  14 ib_iser,ib_cm,rdma_cm,ib_umad,ib_srp,iw_cx=
gb4,ib_isert,ib_uverbs,rpcrdma,ib_ipoib,iw_cm,ib_srpt,ib_ucm,rdma_ucm
libcxgb                16384  1 iw_cxgb4
cxgb4                 282624  1 iw_cxgb4
ptp                    20480  2 cxgb4,e1000e

[root@t5nic ~]# ifconfig -a | grep -i 06:44 | wc -l
64

3>Unloaded cxgb4vf on Host, and verified in lsmod

[root@t5nic ~]# rmmod cxgb4vf

[root@t5nic ~]#  lsmod | grep -i cxgb4
iw_cxgb4              204800  0
ib_core               212992  14 ib_iser,ib_cm,rdma_cm,ib_umad,ib_srp,iw_cx=
gb4,ib_isert,ib_uverbs,rpcrdma,ib_ipoib,iw_cm,ib_srpt,ib_ucm,rdma_ucm
libcxgb                16384  1 iw_cxgb4
cxgb4                 282624  1 iw_cxgb4
ptp                    20480  2 cxgb4,e1000e

[root@t5nic ~]# ifconfig -a | grep -i 06:44 | wc -l
0

4>Attached one VF to the VM and powered on the VM. observed cxgb4vf is
getting automatically loaded on the Host. And observing 18 VF interfaces on
Host out of 63.

[root@t5nic ~]# lsmod | grep -i cxgb4
cxgb4vf                73728  0
iw_cxgb4              204800  0
ib_core               212992  14 ib_iser,ib_cm,rdma_cm,ib_umad,ib_srp,iw_cx=
gb4,ib_isert,ib_uverbs,rpcrdma,ib_ipoib,iw_cm,ib_srpt,ib_ucm,rdma_ucm
libcxgb                16384  1 iw_cxgb4
cxgb4                 282624  1 iw_cxgb4
ptp                    20480  2 cxgb4,e1000e

[root@t5nic ~]# ifconfig -a | grep -i 06:44 | wc -l
18
------------

  Komali's has tried this on 4.13.7 and not observed the above behavior.  W=
e
only see cxgb4vf loaded when the SR-IOV Virtual Functions are first
instantiated.  And it's interesting that the PCI layer only bound the
reloaded cxgb4vf to 18 of the 64 when she powered up the VM.  If the kernel
was going ahead and signalling the reload of cxgb4vf and was fencing off th=
e
one VF it attached to a VM, I would have expected cxgb4vf to be bound to th=
e
63 remainign VFs ...

Casey

Re: 4.14.0-rc3 is auto-reloading PCIe SR-IOV Virtual Function device drivers when VFs are attached to VMs ...

[Casey Leedom]

| From: Luis R. Rodriguez <mcgrof@kernel.org>
| Sent: Tuesday, October 17, 2017 6:14:20 PM
|
| You referred to commit 2355869. What tree and more importantly what patch
| title?

  Ah, my bad.  Sorry, I had the impression that git commit IDs were
preserved across repositories.  That's in Linus' main repository:

  commit 235586939d7fe4833ada9e988f92af543ee6851f
  Author: Luis R. Rodriguez <mcgrof@kernel.org>
  Date:   Fri Sep 8 16:17:00 2017 -0700

    kmod: split out umh code into its own file

    Patch series "kmod: few code cleanups to split out umh code"

    The usermode helper has a provenance from the old usb code which first
    required a usermode helper.  Eventually this was shoved into kmod.c and
    the kernel's modprobe calls was converted over eventually to share the
    same code.  Over time the list of usermode helpers in the kernel has gr=
own
    -- so kmod is just but one user of the API.
    ...

But, that was just a random guess since it was the largest recent change in
this area.  Komali tried to apply a reverse patch to see if that guess
worked out but that didn't apply cleanly and trying to reset to 2355869^ le=
d
to other problems with systemd-udev.service running constantly reloading th=
e
driver with a RHEL7 installation ... which may be a hint about what's going
on ...  I'm going to recommend that Komali do a bisect to see if she can
figure out where this new behavior started ...

Casey

Re: 4.14.0-rc3 is auto-reloading PCIe SR-IOV Virtual Function device drivers when VFs are attached to VMs ...

[Luis R. Rodriguez]

On Wed, Oct 18, 2017 at 10:09 AM, Casey Leedom <leedom@chelsio.com> wrote:
> | From: Luis R. Rodriguez <mcgrof@kernel.org>
> | Sent: Tuesday, October 17, 2017 6:14:20 PM
> |
> | You referred to commit 2355869. What tree and more importantly what patch
> | title?
>
>   Ah, my bad.  Sorry, I had the impression that git commit IDs were
> preserved across repositories.  That's in Linus' main repository:
>
>   commit 235586939d7fe4833ada9e988f92af543ee6851f
>   Author: Luis R. Rodriguez <mcgrof@kernel.org>
>   Date:   Fri Sep 8 16:17:00 2017 -0700
>
>     kmod: split out umh code into its own file
>
>     Patch series "kmod: few code cleanups to split out umh code"
>
>     The usermode helper has a provenance from the old usb code which first
>     required a usermode helper.  Eventually this was shoved into kmod.c and
>     the kernel's modprobe calls was converted over eventually to share the
>     same code.  Over time the list of usermode helpers in the kernel has grown
>     -- so kmod is just but one user of the API.
>     ...
>
> But, that was just a random guess since it was the largest recent change in
> this area.  Komali tried to apply a reverse patch to see if that guess
> worked out but that didn't apply cleanly and trying to reset to 2355869^ led
> to other problems with systemd-udev.service running constantly reloading the
> driver with a RHEL7 installation ... which may be a hint about what's going
> on ...  I'm going to recommend that Komali do a bisect to see if she can
> figure out where this new behavior started ...

I'd recommend to do a full bisect, I *highly* doubt the above commit
is the issue given it really should not have introduced any functional
changes as its just a code shuffle, moving code from one file to
another. Specially for the type of change you are describing, that
requires significant changes. So save your time and just focus on a
proper bisect.

  Luis

Re: 4.14.0-rc3 is auto-reloading PCIe SR-IOV Virtual Function device drivers when VFs are attached to VMs ...

[Casey Leedom]

  Okay, I'll have Komali do a full bisct and report back.

Casey

Re: 4.14.0-rc3 is auto-reloading PCIe SR-IOV Virtual Function device drivers when VFs are attached to VMs ...

[Bjorn Helgaas]

On Wed, Oct 18, 2017 at 05:09:33PM +0000, Casey Leedom wrote:
> | From: Luis R. Rodriguez <mcgrof@kernel.org>
> | Sent: Tuesday, October 17, 2017 6:14:20 PM
> |
> | You referred to commit 2355869. What tree and more importantly what patch
> | title?
> 
>   Ah, my bad.  Sorry, I had the impression that git commit IDs were
> preserved across repositories.  That's in Linus' main repository:
> 
>   commit 235586939d7fe4833ada9e988f92af543ee6851f

Oh, sorry!  You *did* provide the git SHA1 already.  I just didn't
recognize it because it as one because it happened to contain no hex
digits (a-f) in the seven characters you quoted.  I wonder what the
chances of that are.

Re: 4.14.0-rc3 is auto-reloading PCIe SR-IOV Virtual Function device drivers when VFs are attached to VMs ...

[Casey Leedom]

| From: Bjorn Helgaas <helgaas@kernel.org>
| Sent: Wednesday, October 18, 2017 11:02 AM
|  =20
| On Wed, Oct 18, 2017 at 05:09:33PM +0000, Casey Leedom wrote:
| > | From: Luis R. Rodriguez <mcgrof@kernel.org>
| > | Sent: Tuesday, October 17, 2017 6:14:20 PM
| > |
| > | You referred to commit 2355869. What tree and more importantly what
| > | patch title?
| >=20
| >   Ah, my bad.  Sorry, I had the impression that git commit IDs were
| > preserved across repositories.  That's in Linus' main repository:
| >=20
| >   commit 235586939d7fe4833ada9e988f92af543ee6851f
|=20
| Oh, sorry!  You *did* provide the git SHA1 already.  I just didn't
| recognize it because it as one because it happened to contain no hex
| digits (a-f) in the seven characters you quoted.  I wonder what the
| chances of that are.

  10^7 / 16^7 ~=3D 3.7%

Casey

Re: 4.14.0-rc3 is auto-reloading PCIe SR-IOV Virtual Function device drivers when VFs are attached to VMs ...

[Casey Leedom]

/ From: Casey Leedom <leedom@chelsio.com>
| Date: Tuesday, October 17, 2017 11:36 AM
|=20
|   I hope this is the right Linux list for this issue.  One of our QA staf=
f
| has noticed a new behavior starting about 4.14.0-rc3.  We instantiate PCI=
e
| SR-IOV Virtual Functions and the corresponding driver (cxgb4vf in this ca=
se)
| is automatically loaded.  That's fine and has been the case for some time=
.
| But, we remove the driver module (rmmod cxgb4vf) and then attach one of t=
he
| VFs to a KVM Virtual Machine and the cxgb4vf driver module gets reloaded =
in
| the KVM Hypervisor.  This is new behavior.  I see that there was a pretty
| big change done by Luis R. Rodriguez in kernel.org commit revision 235586=
9
| but we haven't yet isolated the new behavior to that commit.  That'll be =
our
\ next test but I wanted to get this out there for comment.

...

/ From: Casey Leedom <leedom@chelsio.com>
| Date: Wednesday, October 18, 2017 10:09 AM
|=20
|   Ah, my bad.  Sorry, I had the impression that git commit IDs were
| preserved across repositories.  That's in Linus' main repository:
|=20
|   commit 235586939d7fe4833ada9e988f92af543ee6851f
|   Author: Luis R. Rodriguez <mcgrof@kernel.org>
|   Date:   Fri Sep 8 16:17:00 2017 -0700
|=20
|     kmod: split out umh code into its own file
|=20
|     Patch series "kmod: few code cleanups to split out umh code"
|=20
|     The usermode helper has a provenance from the old usb code which firs=
t
|     required a usermode helper.  Eventually this was shoved into kmod.c a=
nd
|     the kernel's modprobe calls was converted over eventually to share th=
e
|     same code.  Over time the list of usermode helpers in the kernel has =
grown
|     -- so kmod is just but one user of the API.
|     ...
|=20
| But, that was just a random guess since it was the largest recent change =
in
| this area.  Komali tried to apply a reverse patch to see if that guess
| worked out but that didn't apply cleanly and trying to reset to 2355869^ =
led
| to other problems with systemd-udev.service running constantly reloading =
the
| driver with a RHEL7 installation ... which may be a hint about what's goi=
ng
| on ...  I'm going to recommend that Komali do a bisect to see if she can
\ figure out where this new behavior started ...


/ From:  Luis R. Rodriguez <mcgrof@kernel.org>
| Date: Wednesday, October 18, 2017 10:18 AM
|=20
| I'd recommend to do a full bisect, I *highly* doubt the above commit is t=
he
| issue given it really should not have introduced any functional changes a=
s
| its just a code shuffle, moving code from one file to another. Specially =
for
| the type of change you are describing, that requires significant changes.=
 So
\ save your time and just focus on a proper bisect.


/ From: Casey Leedom
| Date: Wednesday, October 18, 2017 10:23 AM
|=20
\   Okay, I'll have Komali do a full bisct and report back.


  Sorry for the incredibly late response to this -- we've all been busy wit=
h
release schedules, etc.  I hope it's okay that I included a significant
amount of the previous message context -- I figured that it had been so lon=
g
that it would be better to bring everyone back up to speed.

  In any case, Komali did the bisect and identified the commit which has
resulted in the new behavior which is causing problems:

  commit 1455cf8dbfd06aa7651dcfccbadb7a093944ca65
  Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
  Date:   Wed Jul 19 17:24:30 2017 -0700

    driver core: emit uevents when device is bound to a driver

    There are certain touch controllers that may come up in either normal
    (application) or boot mode, depending on whether firmware/configuration=
 is
    corrupted when they are powered on. In boot mode the kernel does not cr=
eate
    input device instance (because it does not necessarily know the
    characteristics of the input device in question).

    Another number of controllers does not store firmware in a non-volatile
    memory, and they similarly need to have firmware loaded before input de=
vice
    instance is created. There are also other types of devices with similar
    behavior.

    There is a desire to be able to trigger firmware loading via udev, but =
it
    has to happen only when driver is bound to a physical device (i2c or sp=
i).
    These udev actions can not use ADD events, as those happen too early, s=
o we
    are introducing BIND and UNBIND events that are emitted at the right
    moment.

    Also, many drivers create additional driver-specific device attributes
    when binding to the device, to provide userspace with additional contro=
ls.
    The new events allow userspace to adjust these driver-specific attribut=
es
    without worrying that they are not there yet.

    Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

I've Cc'ed both Dmitry and Greg on this message to get their feedback on th=
is.

Casey

Re: 4.14.0-rc3 is auto-reloading PCIe SR-IOV Virtual Function device drivers when VFs are attached to VMs ...

[Dmitry Torokhov]

On Wed, Dec 13, 2017 at 1:33 PM, Casey Leedom <leedom@chelsio.com> wrote:
> / From: Casey Leedom <leedom@chelsio.com>
> | Date: Tuesday, October 17, 2017 11:36 AM
> |
> |   I hope this is the right Linux list for this issue.  One of our QA staff
> | has noticed a new behavior starting about 4.14.0-rc3.  We instantiate PCIe
> | SR-IOV Virtual Functions and the corresponding driver (cxgb4vf in this case)
> | is automatically loaded.  That's fine and has been the case for some time.
> | But, we remove the driver module (rmmod cxgb4vf) and then attach one of the
> | VFs to a KVM Virtual Machine and the cxgb4vf driver module gets reloaded in
> | the KVM Hypervisor.  This is new behavior.  I see that there was a pretty
> | big change done by Luis R. Rodriguez in kernel.org commit revision 2355869
> | but we haven't yet isolated the new behavior to that commit.  That'll be our
> \ next test but I wanted to get this out there for comment.
>
> ...
>
> / From: Casey Leedom <leedom@chelsio.com>
> | Date: Wednesday, October 18, 2017 10:09 AM
> |
> |   Ah, my bad.  Sorry, I had the impression that git commit IDs were
> | preserved across repositories.  That's in Linus' main repository:
> |
> |   commit 235586939d7fe4833ada9e988f92af543ee6851f
> |   Author: Luis R. Rodriguez <mcgrof@kernel.org>
> |   Date:   Fri Sep 8 16:17:00 2017 -0700
> |
> |     kmod: split out umh code into its own file
> |
> |     Patch series "kmod: few code cleanups to split out umh code"
> |
> |     The usermode helper has a provenance from the old usb code which first
> |     required a usermode helper.  Eventually this was shoved into kmod.c and
> |     the kernel's modprobe calls was converted over eventually to share the
> |     same code.  Over time the list of usermode helpers in the kernel has grown
> |     -- so kmod is just but one user of the API.
> |     ...
> |
> | But, that was just a random guess since it was the largest recent change in
> | this area.  Komali tried to apply a reverse patch to see if that guess
> | worked out but that didn't apply cleanly and trying to reset to 2355869^ led
> | to other problems with systemd-udev.service running constantly reloading the
> | driver with a RHEL7 installation ... which may be a hint about what's going
> | on ...  I'm going to recommend that Komali do a bisect to see if she can
> \ figure out where this new behavior started ...
>
>
> / From:  Luis R. Rodriguez <mcgrof@kernel.org>
> | Date: Wednesday, October 18, 2017 10:18 AM
> |
> | I'd recommend to do a full bisect, I *highly* doubt the above commit is the
> | issue given it really should not have introduced any functional changes as
> | its just a code shuffle, moving code from one file to another. Specially for
> | the type of change you are describing, that requires significant changes. So
> \ save your time and just focus on a proper bisect.
>
>
> / From: Casey Leedom
> | Date: Wednesday, October 18, 2017 10:23 AM
> |
> \   Okay, I'll have Komali do a full bisct and report back.
>
>
>   Sorry for the incredibly late response to this -- we've all been busy with
> release schedules, etc.  I hope it's okay that I included a significant
> amount of the previous message context -- I figured that it had been so long
> that it would be better to bring everyone back up to speed.
>
>   In any case, Komali did the bisect and identified the commit which has
> resulted in the new behavior which is causing problems:
>
>   commit 1455cf8dbfd06aa7651dcfccbadb7a093944ca65
>   Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>   Date:   Wed Jul 19 17:24:30 2017 -0700
>
>     driver core: emit uevents when device is bound to a driver
>
>     There are certain touch controllers that may come up in either normal
>     (application) or boot mode, depending on whether firmware/configuration is
>     corrupted when they are powered on. In boot mode the kernel does not create
>     input device instance (because it does not necessarily know the
>     characteristics of the input device in question).
>
>     Another number of controllers does not store firmware in a non-volatile
>     memory, and they similarly need to have firmware loaded before input device
>     instance is created. There are also other types of devices with similar
>     behavior.
>
>     There is a desire to be able to trigger firmware loading via udev, but it
>     has to happen only when driver is bound to a physical device (i2c or spi).
>     These udev actions can not use ADD events, as those happen too early, so we
>     are introducing BIND and UNBIND events that are emitted at the right
>     moment.
>
>     Also, many drivers create additional driver-specific device attributes
>     when binding to the device, to provide userspace with additional controls.
>     The new events allow userspace to adjust these driver-specific attributes
>     without worrying that they are not there yet.
>
>     Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>     Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> I've Cc'ed both Dmitry and Greg on this message to get their feedback on this.
>

This should have been fixed with:

commit 6878e7de6af726de47f9f3bec649c3f49e786586
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date:   Wed Sep 13 16:29:48 2017 -0700

   driver core: suppress sending MODALIAS in UNBIND uevents

   The current udev rules cause modules to be loaded on all device events save
   for "remove". With the introduction of KOBJ_BIND/KOBJ_UNBIND this causes
   issues, as driver modules that have devices bound to their drivers get
   immediately reloaded, and it appears to the user that module unloading doe
   snot work.

   The standard udev matching rule is foillowing:

   ENV{MODALIAS}=="?*", RUN{builtin}+="kmod load $env{MODALIAS}"

   Given that MODALIAS data is not terribly useful for UNBIND event, let's zap
   it from the generated uevent environment until we get userspace updated
   with the correct udev rule that only loads modules on "add" event.

   Reported-by: Jakub Kicinski <kubakici@wp.pl>
   Tested-by: Jakub Kicinski <kubakici@wp.pl>
   Fixes: 1455cf8dbfd0 ("driver core: emit uevents when device is bound ...")
   Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
   Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

but may have been broken again with:

commit 4a336a23d619e96aef37d4d054cfadcdd1b581ba
Author: Eric Dumazet <edumazet@google.com>
Date:   Tue Sep 19 16:27:04 2017 -0700

   kobject: copy env blob in one go

   No need to iterate over strings, just copy in one efficient memcpy() call.

CCing Eric as well.

-- 
Dmitry

Re: 4.14.0-rc3 is auto-reloading PCIe SR-IOV Virtual Function device drivers when VFs are attached to VMs ...

[Eric Dumazet]

[-- Attachment #1: Type: text/plain, Size: 6944 bytes --]

On Wed, Dec 13, 2017 at 1:47 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, Dec 13, 2017 at 1:33 PM, Casey Leedom <leedom@chelsio.com> wrote:
>> / From: Casey Leedom <leedom@chelsio.com>
>> | Date: Tuesday, October 17, 2017 11:36 AM
>> |
>> |   I hope this is the right Linux list for this issue.  One of our QA staff
>> | has noticed a new behavior starting about 4.14.0-rc3.  We instantiate PCIe
>> | SR-IOV Virtual Functions and the corresponding driver (cxgb4vf in this case)
>> | is automatically loaded.  That's fine and has been the case for some time.
>> | But, we remove the driver module (rmmod cxgb4vf) and then attach one of the
>> | VFs to a KVM Virtual Machine and the cxgb4vf driver module gets reloaded in
>> | the KVM Hypervisor.  This is new behavior.  I see that there was a pretty
>> | big change done by Luis R. Rodriguez in kernel.org commit revision 2355869
>> | but we haven't yet isolated the new behavior to that commit.  That'll be our
>> \ next test but I wanted to get this out there for comment.
>>
>> ...
>>
>> / From: Casey Leedom <leedom@chelsio.com>
>> | Date: Wednesday, October 18, 2017 10:09 AM
>> |
>> |   Ah, my bad.  Sorry, I had the impression that git commit IDs were
>> | preserved across repositories.  That's in Linus' main repository:
>> |
>> |   commit 235586939d7fe4833ada9e988f92af543ee6851f
>> |   Author: Luis R. Rodriguez <mcgrof@kernel.org>
>> |   Date:   Fri Sep 8 16:17:00 2017 -0700
>> |
>> |     kmod: split out umh code into its own file
>> |
>> |     Patch series "kmod: few code cleanups to split out umh code"
>> |
>> |     The usermode helper has a provenance from the old usb code which first
>> |     required a usermode helper.  Eventually this was shoved into kmod.c and
>> |     the kernel's modprobe calls was converted over eventually to share the
>> |     same code.  Over time the list of usermode helpers in the kernel has grown
>> |     -- so kmod is just but one user of the API.
>> |     ...
>> |
>> | But, that was just a random guess since it was the largest recent change in
>> | this area.  Komali tried to apply a reverse patch to see if that guess
>> | worked out but that didn't apply cleanly and trying to reset to 2355869^ led
>> | to other problems with systemd-udev.service running constantly reloading the
>> | driver with a RHEL7 installation ... which may be a hint about what's going
>> | on ...  I'm going to recommend that Komali do a bisect to see if she can
>> \ figure out where this new behavior started ...
>>
>>
>> / From:  Luis R. Rodriguez <mcgrof@kernel.org>
>> | Date: Wednesday, October 18, 2017 10:18 AM
>> |
>> | I'd recommend to do a full bisect, I *highly* doubt the above commit is the
>> | issue given it really should not have introduced any functional changes as
>> | its just a code shuffle, moving code from one file to another. Specially for
>> | the type of change you are describing, that requires significant changes. So
>> \ save your time and just focus on a proper bisect.
>>
>>
>> / From: Casey Leedom
>> | Date: Wednesday, October 18, 2017 10:23 AM
>> |
>> \   Okay, I'll have Komali do a full bisct and report back.
>>
>>
>>   Sorry for the incredibly late response to this -- we've all been busy with
>> release schedules, etc.  I hope it's okay that I included a significant
>> amount of the previous message context -- I figured that it had been so long
>> that it would be better to bring everyone back up to speed.
>>
>>   In any case, Komali did the bisect and identified the commit which has
>> resulted in the new behavior which is causing problems:
>>
>>   commit 1455cf8dbfd06aa7651dcfccbadb7a093944ca65
>>   Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>   Date:   Wed Jul 19 17:24:30 2017 -0700
>>
>>     driver core: emit uevents when device is bound to a driver
>>
>>     There are certain touch controllers that may come up in either normal
>>     (application) or boot mode, depending on whether firmware/configuration is
>>     corrupted when they are powered on. In boot mode the kernel does not create
>>     input device instance (because it does not necessarily know the
>>     characteristics of the input device in question).
>>
>>     Another number of controllers does not store firmware in a non-volatile
>>     memory, and they similarly need to have firmware loaded before input device
>>     instance is created. There are also other types of devices with similar
>>     behavior.
>>
>>     There is a desire to be able to trigger firmware loading via udev, but it
>>     has to happen only when driver is bound to a physical device (i2c or spi).
>>     These udev actions can not use ADD events, as those happen too early, so we
>>     are introducing BIND and UNBIND events that are emitted at the right
>>     moment.
>>
>>     Also, many drivers create additional driver-specific device attributes
>>     when binding to the device, to provide userspace with additional controls.
>>     The new events allow userspace to adjust these driver-specific attributes
>>     without worrying that they are not there yet.
>>
>>     Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>     Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>
>> I've Cc'ed both Dmitry and Greg on this message to get their feedback on this.
>>
>
> This should have been fixed with:
>
> commit 6878e7de6af726de47f9f3bec649c3f49e786586
> Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Date:   Wed Sep 13 16:29:48 2017 -0700
>
>    driver core: suppress sending MODALIAS in UNBIND uevents
>
>    The current udev rules cause modules to be loaded on all device events save
>    for "remove". With the introduction of KOBJ_BIND/KOBJ_UNBIND this causes
>    issues, as driver modules that have devices bound to their drivers get
>    immediately reloaded, and it appears to the user that module unloading doe
>    snot work.
>
>    The standard udev matching rule is foillowing:
>
>    ENV{MODALIAS}=="?*", RUN{builtin}+="kmod load $env{MODALIAS}"
>
>    Given that MODALIAS data is not terribly useful for UNBIND event, let's zap
>    it from the generated uevent environment until we get userspace updated
>    with the correct udev rule that only loads modules on "add" event.
>
>    Reported-by: Jakub Kicinski <kubakici@wp.pl>
>    Tested-by: Jakub Kicinski <kubakici@wp.pl>
>    Fixes: 1455cf8dbfd0 ("driver core: emit uevents when device is bound ...")
>    Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> but may have been broken again with:
>
> commit 4a336a23d619e96aef37d4d054cfadcdd1b581ba
> Author: Eric Dumazet <edumazet@google.com>
> Date:   Tue Sep 19 16:27:04 2017 -0700
>
>    kobject: copy env blob in one go
>
>    No need to iterate over strings, just copy in one efficient memcpy() call.
>
> CCing Eric as well.
>
> --
> Dmitry

I am about to send this fix :

[-- Attachment #2: patch_zap_modalias_env.txt --]
[-- Type: text/plain, Size: 936 bytes --]

diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index c3e84edc47c965d40199b652ba78876cdaa9c70c..0795482b15d5a8f1b65b570a071aa1419cb923d8 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -346,19 +346,25 @@ static int kobject_uevent_net_broadcast(struct kobject *kobj,
 static void zap_modalias_env(struct kobj_uevent_env *env)
 {
 	static const char modalias_prefix[] = "MODALIAS=";
+	size_t offset = 0, len;
 	int i;
 
 	for (i = 0; i < env->envp_idx;) {
+		len = strlen(env->envp[i]) + 1;
 		if (strncmp(env->envp[i], modalias_prefix,
 			    sizeof(modalias_prefix) - 1)) {
 			i++;
+			offset += len;
 			continue;
 		}
 
-		if (i != env->envp_idx - 1)
+		env->buflen -= len;
+		if (i != env->envp_idx - 1) {
+			memmove(env->envp[i], env->envp[i + 1],
+				env->buflen - offset);
 			memmove(&env->envp[i], &env->envp[i + 1],
 				sizeof(env->envp[i]) * env->envp_idx - 1);
-
+		}
 		env->envp_idx--;
 	}
 }

Re: 4.14.0-rc3 is auto-reloading PCIe SR-IOV Virtual Function device drivers when VFs are attached to VMs ...

[Dmitry Torokhov]

On Wed, Dec 13, 2017 at 1:54 PM, Eric Dumazet <edumazet@google.com> wrote:
> On Wed, Dec 13, 2017 at 1:47 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> On Wed, Dec 13, 2017 at 1:33 PM, Casey Leedom <leedom@chelsio.com> wrote:
>>> / From: Casey Leedom <leedom@chelsio.com>
>>> | Date: Tuesday, October 17, 2017 11:36 AM
>>> |
>>> |   I hope this is the right Linux list for this issue.  One of our QA staff
>>> | has noticed a new behavior starting about 4.14.0-rc3.  We instantiate PCIe
>>> | SR-IOV Virtual Functions and the corresponding driver (cxgb4vf in this case)
>>> | is automatically loaded.  That's fine and has been the case for some time.
>>> | But, we remove the driver module (rmmod cxgb4vf) and then attach one of the
>>> | VFs to a KVM Virtual Machine and the cxgb4vf driver module gets reloaded in
>>> | the KVM Hypervisor.  This is new behavior.  I see that there was a pretty
>>> | big change done by Luis R. Rodriguez in kernel.org commit revision 2355869
>>> | but we haven't yet isolated the new behavior to that commit.  That'll be our
>>> \ next test but I wanted to get this out there for comment.
>>>
>>> ...
>>>
>>> / From: Casey Leedom <leedom@chelsio.com>
>>> | Date: Wednesday, October 18, 2017 10:09 AM
>>> |
>>> |   Ah, my bad.  Sorry, I had the impression that git commit IDs were
>>> | preserved across repositories.  That's in Linus' main repository:
>>> |
>>> |   commit 235586939d7fe4833ada9e988f92af543ee6851f
>>> |   Author: Luis R. Rodriguez <mcgrof@kernel.org>
>>> |   Date:   Fri Sep 8 16:17:00 2017 -0700
>>> |
>>> |     kmod: split out umh code into its own file
>>> |
>>> |     Patch series "kmod: few code cleanups to split out umh code"
>>> |
>>> |     The usermode helper has a provenance from the old usb code which first
>>> |     required a usermode helper.  Eventually this was shoved into kmod.c and
>>> |     the kernel's modprobe calls was converted over eventually to share the
>>> |     same code.  Over time the list of usermode helpers in the kernel has grown
>>> |     -- so kmod is just but one user of the API.
>>> |     ...
>>> |
>>> | But, that was just a random guess since it was the largest recent change in
>>> | this area.  Komali tried to apply a reverse patch to see if that guess
>>> | worked out but that didn't apply cleanly and trying to reset to 2355869^ led
>>> | to other problems with systemd-udev.service running constantly reloading the
>>> | driver with a RHEL7 installation ... which may be a hint about what's going
>>> | on ...  I'm going to recommend that Komali do a bisect to see if she can
>>> \ figure out where this new behavior started ...
>>>
>>>
>>> / From:  Luis R. Rodriguez <mcgrof@kernel.org>
>>> | Date: Wednesday, October 18, 2017 10:18 AM
>>> |
>>> | I'd recommend to do a full bisect, I *highly* doubt the above commit is the
>>> | issue given it really should not have introduced any functional changes as
>>> | its just a code shuffle, moving code from one file to another. Specially for
>>> | the type of change you are describing, that requires significant changes. So
>>> \ save your time and just focus on a proper bisect.
>>>
>>>
>>> / From: Casey Leedom
>>> | Date: Wednesday, October 18, 2017 10:23 AM
>>> |
>>> \   Okay, I'll have Komali do a full bisct and report back.
>>>
>>>
>>>   Sorry for the incredibly late response to this -- we've all been busy with
>>> release schedules, etc.  I hope it's okay that I included a significant
>>> amount of the previous message context -- I figured that it had been so long
>>> that it would be better to bring everyone back up to speed.
>>>
>>>   In any case, Komali did the bisect and identified the commit which has
>>> resulted in the new behavior which is causing problems:
>>>
>>>   commit 1455cf8dbfd06aa7651dcfccbadb7a093944ca65
>>>   Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>   Date:   Wed Jul 19 17:24:30 2017 -0700
>>>
>>>     driver core: emit uevents when device is bound to a driver
>>>
>>>     There are certain touch controllers that may come up in either normal
>>>     (application) or boot mode, depending on whether firmware/configuration is
>>>     corrupted when they are powered on. In boot mode the kernel does not create
>>>     input device instance (because it does not necessarily know the
>>>     characteristics of the input device in question).
>>>
>>>     Another number of controllers does not store firmware in a non-volatile
>>>     memory, and they similarly need to have firmware loaded before input device
>>>     instance is created. There are also other types of devices with similar
>>>     behavior.
>>>
>>>     There is a desire to be able to trigger firmware loading via udev, but it
>>>     has to happen only when driver is bound to a physical device (i2c or spi).
>>>     These udev actions can not use ADD events, as those happen too early, so we
>>>     are introducing BIND and UNBIND events that are emitted at the right
>>>     moment.
>>>
>>>     Also, many drivers create additional driver-specific device attributes
>>>     when binding to the device, to provide userspace with additional controls.
>>>     The new events allow userspace to adjust these driver-specific attributes
>>>     without worrying that they are not there yet.
>>>
>>>     Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>     Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>
>>> I've Cc'ed both Dmitry and Greg on this message to get their feedback on this.
>>>
>>
>> This should have been fixed with:
>>
>> commit 6878e7de6af726de47f9f3bec649c3f49e786586
>> Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Date:   Wed Sep 13 16:29:48 2017 -0700
>>
>>    driver core: suppress sending MODALIAS in UNBIND uevents
>>
>>    The current udev rules cause modules to be loaded on all device events save
>>    for "remove". With the introduction of KOBJ_BIND/KOBJ_UNBIND this causes
>>    issues, as driver modules that have devices bound to their drivers get
>>    immediately reloaded, and it appears to the user that module unloading doe
>>    snot work.
>>
>>    The standard udev matching rule is foillowing:
>>
>>    ENV{MODALIAS}=="?*", RUN{builtin}+="kmod load $env{MODALIAS}"
>>
>>    Given that MODALIAS data is not terribly useful for UNBIND event, let's zap
>>    it from the generated uevent environment until we get userspace updated
>>    with the correct udev rule that only loads modules on "add" event.
>>
>>    Reported-by: Jakub Kicinski <kubakici@wp.pl>
>>    Tested-by: Jakub Kicinski <kubakici@wp.pl>
>>    Fixes: 1455cf8dbfd0 ("driver core: emit uevents when device is bound ...")
>>    Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>
>> but may have been broken again with:
>>
>> commit 4a336a23d619e96aef37d4d054cfadcdd1b581ba
>> Author: Eric Dumazet <edumazet@google.com>
>> Date:   Tue Sep 19 16:27:04 2017 -0700
>>
>>    kobject: copy env blob in one go
>>
>>    No need to iterate over strings, just copy in one efficient memcpy() call.
>>
>> CCing Eric as well.
>>
>> --
>> Dmitry
>
> I am about to send this fix :

Don't you need to update individual pointers if you are altering the buffer?

-- 
Dmitry

Re: 4.14.0-rc3 is auto-reloading PCIe SR-IOV Virtual Function device drivers when VFs are attached to VMs ...

[Eric Dumazet]

> Don't you need to update individual pointers if you are altering the buffer?
>

Possibly...

Re: 4.14.0-rc3 is auto-reloading PCIe SR-IOV Virtual Function device drivers when VFs are attached to VMs ...

[Eric Dumazet]

On Wed, Dec 13, 2017 at 2:06 PM, Eric Dumazet <edumazet@google.com> wrote:
>> Don't you need to update individual pointers if you are altering the buffer?
>>
>
> Possibly...

I am packing my stuff, before a long flight.

I wont be able to fix this before at least a day, so feel free to fix
this patch ;)

Thanks.

Re: 4.14.0-rc3 is auto-reloading PCIe SR-IOV Virtual Function device drivers when VFs are attached to VMs ...

[Casey Leedom]

/ From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
| Sent: Wednesday, December 13, 2017 1:47 PM
|
| This should have been fixed with:
|
|   commit 6878e7de6af726de47f9f3bec649c3f49e786586
|   Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
\   Date:   Wed Sep 13 16:29:48 2017 -0700

  I see that was committed on September 13, 2017.  I should have mentioned
that Komali started her bisect with kernel.org commit
a638349bf6c29433b938141f99225b160551ff48 which is dated December 11, 2017.
So something more needs to be done ...

  It looks like you and Eric are discussing an additional fix.  If you come
up with a patch, we [Komali] can test it for you.

Casey