[PATCH v2] kobject: add kernel/uevent_features sysfs file

[PATCH v2] kobject: add kernel/uevent_features sysfs file

[Peter Rajnoha]

We can use extended format when writing /sys/.../uevent files to
generate synthetic uevents, introduced with commit f36776fafbaa
("kobject: support passing in variables for synthetic uevents").

Before using this extended format, we need to know if it's supported
and kernel version check may not be appropriate in all cases - there
are possible differences from upstream kernel in distributions with
backports.

This patch adds /sys/kernel/uevent_features file which currently lists
'synthargs' string to denote that the kernel is able to recognize the
extended synthetic uevent arguments. Userspace can easily check for
the feature then.

Updates for v2:
        Add Documentation/ABI/testing/sysfs-kernel-uevent_features.

Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
---
 .../ABI/testing/sysfs-kernel-uevent_features         | 12 ++++++++++++
 kernel/ksysfs.c                                      |  8 ++++++++
 2 files changed, 20 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-kernel-uevent_features

diff --git a/Documentation/ABI/testing/sysfs-kernel-uevent_features b/Documentation/ABI/testing/sysfs-kernel-uevent_features
new file mode 100644
index 000000000000..10b1d07c5ef9
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-kernel-uevent_features
@@ -0,0 +1,12 @@
+What:           /sys/kernel/uevent_features
+Date:           December 2018
+KernelVersion:  4.21
+Contact:        Linux kernel mailing list <linux-kernel@vger.kernel.org>
+Description:
+                Reading from this file returns space-separated list of
+                supported uevent features in current kernel.
+
+                Possible values:
+                        synthargs: passing additional variables for synthetic
+                                   uevents is supported (see also related
+                                   sysfs-uevent ABI documentation)
diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
index 46ba853656f6..d893d7442f61 100644
--- a/kernel/ksysfs.c
+++ b/kernel/ksysfs.c
@@ -37,6 +37,13 @@ static ssize_t uevent_seqnum_show(struct kobject *kobj,
 }
 KERNEL_ATTR_RO(uevent_seqnum);
 
+static ssize_t uevent_features_show(struct kobject *kobj,
+				    struct kobj_attribute *attr, char *buf)
+{
+	return sprintf(buf, "synthargs\n");
+}
+KERNEL_ATTR_RO(uevent_features);
+
 #ifdef CONFIG_UEVENT_HELPER
 /* uevent helper program, used during early boot */
 static ssize_t uevent_helper_show(struct kobject *kobj,
@@ -213,6 +220,7 @@ EXPORT_SYMBOL_GPL(kernel_kobj);
 static struct attribute * kernel_attrs[] = {
 	&fscaps_attr.attr,
 	&uevent_seqnum_attr.attr,
+	&uevent_features_attr.attr,
 #ifdef CONFIG_UEVENT_HELPER
 	&uevent_helper_attr.attr,
 #endif
-- 
2.19.2

Re: [PATCH v2] kobject: add kernel/uevent_features sysfs file

[Greg KH]

On Fri, Dec 07, 2018 at 12:46:07PM +0100, Peter Rajnoha wrote:
> We can use extended format when writing /sys/.../uevent files to
> generate synthetic uevents, introduced with commit f36776fafbaa
> ("kobject: support passing in variables for synthetic uevents").
> 
> Before using this extended format, we need to know if it's supported
> and kernel version check may not be appropriate in all cases - there
> are possible differences from upstream kernel in distributions with
> backports.
> 
> This patch adds /sys/kernel/uevent_features file which currently lists
> 'synthargs' string to denote that the kernel is able to recognize the
> extended synthetic uevent arguments. Userspace can easily check for
> the feature then.

So this is just to try to have userspace detect what type of feature the
kernel has?  Why can't you just go off of the other sysfs file itself?
You shouldn't need a "this is a feature list" for the kernel, otherwise
we would be on a huge slippery slope trying to document everything.

Who is going to use this thing?  And what else would go into it?

Isn't there some other way you can detect this from userspace (like
writing to the file and it fails?)

thanks,

greg k-h

Re: [PATCH v2] kobject: add kernel/uevent_features sysfs file

[Peter Rajnoha]

On 12/7/18 1:01 PM, Greg KH wrote:
> On Fri, Dec 07, 2018 at 12:46:07PM +0100, Peter Rajnoha wrote:
>> This patch adds /sys/kernel/uevent_features file which currently lists
>> 'synthargs' string to denote that the kernel is able to recognize the
>> extended synthetic uevent arguments. Userspace can easily check for
>> the feature then.
> 
> So this is just to try to have userspace detect what type of feature the
> kernel has?  Why can't you just go off of the other sysfs file itself?
> You shouldn't need a "this is a feature list" for the kernel, otherwise
> we would be on a huge slippery slope trying to document everything.
> 
> Who is going to use this thing?  And what else would go into it?
> 
> Isn't there some other way you can detect this from userspace (like
> writing to the file and it fails?)
> 

Yes, it's for userspace to be sure that uevent interface has certain
features available that it can use.

For now, it's just that "synthetic uevent arguments" that is the
extension of the original uevent interface. That applies to both input
(writing to /sys/.../uevent file) and output (related extra variables
that appear in generated uevents).

The obvious user of this is going to be systemd/udev that will add extra
variables to identify various synthetic uevents it produces (coming as
result of the WATCH udev rule, coming from the udevadm trigger call and
other specific uses where it needs to generate synthetic uevents). Other
users I know of involve storage handling tools which need to generate
these synthetic uevents whenever a change happens and it needs to
synchronize with udevd processing (e.g. waiting on refresh to get
reflected in udev database).

I understand that there is an argument that we can just use kernel
version check, but this is not acceptable for all unfortunately (see
also https://github.com/systemd/systemd/pull/7294#issuecomment-343491015).

The issue with checking the return code after writing to /sys/.../uevent
is that it doesn't work with older kernel releases because there, it
always returned success, no matter if the input string was correct or
not or whether the arguments were recognized (unfortunately, this was
like that from beginning, it seems). Even though, I've fixed this return
code with df44b479 recently, but still, there are possible older
releases out there... And still, there might be new variables introduced
in the future that don't necessarily need to be direct result of writing
to /sys/.../uevent file.

-- 
Peter

Re: [PATCH v2] kobject: add kernel/uevent_features sysfs file

[Greg KH]

On Fri, Dec 07, 2018 at 01:28:52PM +0100, Peter Rajnoha wrote:
> On 12/7/18 1:01 PM, Greg KH wrote:
> > On Fri, Dec 07, 2018 at 12:46:07PM +0100, Peter Rajnoha wrote:
> >> This patch adds /sys/kernel/uevent_features file which currently lists
> >> 'synthargs' string to denote that the kernel is able to recognize the
> >> extended synthetic uevent arguments. Userspace can easily check for
> >> the feature then.
> > 
> > So this is just to try to have userspace detect what type of feature the
> > kernel has?  Why can't you just go off of the other sysfs file itself?
> > You shouldn't need a "this is a feature list" for the kernel, otherwise
> > we would be on a huge slippery slope trying to document everything.
> > 
> > Who is going to use this thing?  And what else would go into it?
> > 
> > Isn't there some other way you can detect this from userspace (like
> > writing to the file and it fails?)
> > 
> 
> Yes, it's for userspace to be sure that uevent interface has certain
> features available that it can use.

That is nice, but no, that is not how we export to userspace what
"features" a specific kernel has, sorry.

> For now, it's just that "synthetic uevent arguments" that is the
> extension of the original uevent interface. That applies to both input
> (writing to /sys/.../uevent file) and output (related extra variables
> that appear in generated uevents).
> 
> The obvious user of this is going to be systemd/udev that will add extra
> variables to identify various synthetic uevents it produces (coming as
> result of the WATCH udev rule, coming from the udevadm trigger call and
> other specific uses where it needs to generate synthetic uevents). Other
> users I know of involve storage handling tools which need to generate
> these synthetic uevents whenever a change happens and it needs to
> synchronize with udevd processing (e.g. waiting on refresh to get
> reflected in udev database).
> 
> I understand that there is an argument that we can just use kernel
> version check, but this is not acceptable for all unfortunately (see
> also https://github.com/systemd/systemd/pull/7294#issuecomment-343491015).

Kernel version checks are horrible as well, I know.

> The issue with checking the return code after writing to /sys/.../uevent
> is that it doesn't work with older kernel releases because there, it
> always returned success, no matter if the input string was correct or
> not or whether the arguments were recognized (unfortunately, this was
> like that from beginning, it seems). Even though, I've fixed this return
> code with df44b479 recently, but still, there are possible older
> releases out there... And still, there might be new variables introduced
> in the future that don't necessarily need to be direct result of writing
> to /sys/.../uevent file.

We do not add things to the kernel for "maybe sometime in the future
something else might be added", sorry.  We deal with what we have now.

And right now the kernel is fine, it is userspace that is having a
problem with this.  Why can't you just try to trigger an event from
userspace and if it does not come back, then you know that kernel does
not have that feature?

thanks,
greg k-h

Re: [PATCH v2] kobject: add kernel/uevent_features sysfs file

[Peter Rajnoha]

On 12/19/18 10:24 AM, Greg KH wrote:
> On Fri, Dec 07, 2018 at 01:28:52PM +0100, Peter Rajnoha wrote:
>> On 12/7/18 1:01 PM, Greg KH wrote:
>>> On Fri, Dec 07, 2018 at 12:46:07PM +0100, Peter Rajnoha wrote:
>>>> This patch adds /sys/kernel/uevent_features file which currently lists
>>>> 'synthargs' string to denote that the kernel is able to recognize the
>>>> extended synthetic uevent arguments. Userspace can easily check for
>>>> the feature then.
>>>
>>> So this is just to try to have userspace detect what type of feature the
>>> kernel has?  Why can't you just go off of the other sysfs file itself?
>>> You shouldn't need a "this is a feature list" for the kernel, otherwise
>>> we would be on a huge slippery slope trying to document everything.
>>>
>>> Who is going to use this thing?  And what else would go into it?
>>>
>>> Isn't there some other way you can detect this from userspace (like
>>> writing to the file and it fails?)
>>>
>>
>> Yes, it's for userspace to be sure that uevent interface has certain
>> features available that it can use.
> 
> That is nice, but no, that is not how we export to userspace what
> "features" a specific kernel has, sorry.
> 

I've already seen existing "features" files already in /sys:

  /sys/kernel/cgroup/features
  /sys/fs/ext4/features
  /sys/kernel/debug/sched_features	
  ...

(Though the one under "debug" is a bit different type of coffee, I have
to admit.)

>> For now, it's just that "synthetic uevent arguments" that is the
>> extension of the original uevent interface. That applies to both input
>> (writing to /sys/.../uevent file) and output (related extra variables
>> that appear in generated uevents).
>>
>> The obvious user of this is going to be systemd/udev that will add extra
>> variables to identify various synthetic uevents it produces (coming as
>> result of the WATCH udev rule, coming from the udevadm trigger call and
>> other specific uses where it needs to generate synthetic uevents). Other
>> users I know of involve storage handling tools which need to generate
>> these synthetic uevents whenever a change happens and it needs to
>> synchronize with udevd processing (e.g. waiting on refresh to get
>> reflected in udev database).
>>
>> I understand that there is an argument that we can just use kernel
>> version check, but this is not acceptable for all unfortunately (see
>> also https://github.com/systemd/systemd/pull/7294#issuecomment-343491015).
> 
> Kernel version checks are horrible as well, I know.
> 
>> The issue with checking the return code after writing to /sys/.../uevent
>> is that it doesn't work with older kernel releases because there, it
>> always returned success, no matter if the input string was correct or
>> not or whether the arguments were recognized (unfortunately, this was
>> like that from beginning, it seems). Even though, I've fixed this return
>> code with df44b479 recently, but still, there are possible older
>> releases out there... And still, there might be new variables introduced
>> in the future that don't necessarily need to be direct result of writing
>> to /sys/.../uevent file.
> 
> We do not add things to the kernel for "maybe sometime in the future
> something else might be added", sorry.  We deal with what we have now.
> 
> And right now the kernel is fine, it is userspace that is having a
> problem with this.  Why can't you just try to trigger an event from
> userspace and if it does not come back, then you know that kernel does
> not have that feature?

Because in that case, there's an issue arising of how much should we
wait for the uevent to appear back in userspace after triggering it.
There's no right timeout.

Of course, we wouldn't need to think about all of this if the "write" to
the "uevent" file properly returned error code, but unfortunately it
didn't and that was the bug that was sitting there from day one, it
seems (...fixed now, but still there are those older kernel versions out
there).

-- 
Peter

Re: [PATCH v2] kobject: add kernel/uevent_features sysfs file

[Greg KH]

On Wed, Jan 02, 2019 at 10:59:53AM +0100, Peter Rajnoha wrote:
> On 12/19/18 10:24 AM, Greg KH wrote:
> > On Fri, Dec 07, 2018 at 01:28:52PM +0100, Peter Rajnoha wrote:
> >> On 12/7/18 1:01 PM, Greg KH wrote:
> >>> On Fri, Dec 07, 2018 at 12:46:07PM +0100, Peter Rajnoha wrote:
> >>>> This patch adds /sys/kernel/uevent_features file which currently lists
> >>>> 'synthargs' string to denote that the kernel is able to recognize the
> >>>> extended synthetic uevent arguments. Userspace can easily check for
> >>>> the feature then.
> >>>
> >>> So this is just to try to have userspace detect what type of feature the
> >>> kernel has?  Why can't you just go off of the other sysfs file itself?
> >>> You shouldn't need a "this is a feature list" for the kernel, otherwise
> >>> we would be on a huge slippery slope trying to document everything.
> >>>
> >>> Who is going to use this thing?  And what else would go into it?
> >>>
> >>> Isn't there some other way you can detect this from userspace (like
> >>> writing to the file and it fails?)
> >>>
> >>
> >> Yes, it's for userspace to be sure that uevent interface has certain
> >> features available that it can use.
> > 
> > That is nice, but no, that is not how we export to userspace what
> > "features" a specific kernel has, sorry.
> > 
> 
> I've already seen existing "features" files already in /sys:
> 
>   /sys/kernel/cgroup/features

cgroupfs is "odd", let's not duplicate that :)

>   /sys/fs/ext4/features

One value per file :)

>   /sys/kernel/debug/sched_features	

debugfs does not count, sorry.  We have whole README files in debugfs,
never use that as an excuse for anything outside of debugfs please.

>   ...
> 
> (Though the one under "debug" is a bit different type of coffee, I have
> to admit.)

very different.

Again, I really do not want this in the kernel as it will be a pain to
maintain and support for the next 40+ years just to get over the hump of
the next year when you have a mix of old kernels and new userspace to
deal with.

> >> For now, it's just that "synthetic uevent arguments" that is the
> >> extension of the original uevent interface. That applies to both input
> >> (writing to /sys/.../uevent file) and output (related extra variables
> >> that appear in generated uevents).
> >>
> >> The obvious user of this is going to be systemd/udev that will add extra
> >> variables to identify various synthetic uevents it produces (coming as
> >> result of the WATCH udev rule, coming from the udevadm trigger call and
> >> other specific uses where it needs to generate synthetic uevents). Other
> >> users I know of involve storage handling tools which need to generate
> >> these synthetic uevents whenever a change happens and it needs to
> >> synchronize with udevd processing (e.g. waiting on refresh to get
> >> reflected in udev database).
> >>
> >> I understand that there is an argument that we can just use kernel
> >> version check, but this is not acceptable for all unfortunately (see
> >> also https://github.com/systemd/systemd/pull/7294#issuecomment-343491015).
> > 
> > Kernel version checks are horrible as well, I know.
> > 
> >> The issue with checking the return code after writing to /sys/.../uevent
> >> is that it doesn't work with older kernel releases because there, it
> >> always returned success, no matter if the input string was correct or
> >> not or whether the arguments were recognized (unfortunately, this was
> >> like that from beginning, it seems). Even though, I've fixed this return
> >> code with df44b479 recently, but still, there are possible older
> >> releases out there... And still, there might be new variables introduced
> >> in the future that don't necessarily need to be direct result of writing
> >> to /sys/.../uevent file.
> > 
> > We do not add things to the kernel for "maybe sometime in the future
> > something else might be added", sorry.  We deal with what we have now.
> > 
> > And right now the kernel is fine, it is userspace that is having a
> > problem with this.  Why can't you just try to trigger an event from
> > userspace and if it does not come back, then you know that kernel does
> > not have that feature?
> 
> Because in that case, there's an issue arising of how much should we
> wait for the uevent to appear back in userspace after triggering it.
> There's no right timeout.
> 
> Of course, we wouldn't need to think about all of this if the "write" to
> the "uevent" file properly returned error code, but unfortunately it
> didn't and that was the bug that was sitting there from day one, it
> seems (...fixed now, but still there are those older kernel versions out
> there).

We can backport it to the stable kernels which will then mean that only
any kernel that anyone cares about will result in this getting fixed.
Any systems that do not pick up that change, you can discount as they
will not be getting an updated userspace program either :)

thanks,

greg k-h