On Wed, Dec 13, 2017 at 1:47 PM, Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> wrote:
> On Wed, Dec 13, 2017 at 1:33 PM, Casey Leedom <leedom@xxxxxxxxxxx> wrote:
>> / From: Casey Leedom <leedom@xxxxxxxxxxx>
>> | 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@xxxxxxxxxxx>
>> | 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@xxxxxxxxxx>
>> | 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@xxxxxxxxxx>
>> | 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@xxxxxxxxx>
>> 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@xxxxxxxxx>
>> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxx>
> 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@xxxxx>
> Tested-by: Jakub Kicinski <kubakici@xxxxx>
> Fixes: 1455cf8dbfd0 ("driver core: emit uevents when device is bound ...")
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>
> but may have been broken again with:
>
> commit 4a336a23d619e96aef37d4d054cfadcdd1b581ba
> Author: Eric Dumazet <edumazet@xxxxxxxxxx>
> 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 :
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--;
}
}
