Re: [PATCH net-next] net: ieee802154: Fix compilation error when CONFIG_IEEE802154_NL802154_EXPERIMENTAL is disabled

Re: [PATCH net-next] net: ieee802154: Fix compilation error when CONFIG_IEEE802154_NL802154_EXPERIMENTAL is disabled

[Jakub Kicinski]

On Wed, 31 Aug 2022 09:20:39 +0300 Gal Pressman wrote:
> So, shouldn't it be:
> #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
>         .resv_start_op = NL802154_CMD_DEL_SEC_LEVEL + 1,
> #else
>         .resv_start_op = NL802154_CMD_SET_WPAN_PHY_NETNS + 1,
> #endif

Hm, let me add 802154 folks.

Either we should treat the commands as reserved in terms of uAPI
even if they get removed the IDs won't be reused, or they are for
testing purposes only.

In the former case we should just remove the #ifdef around the values
in the enum, it just leads to #ifdef proliferation while having no
functional impact.

In the latter case we should start error checking from the last
non-experimental command, as we don't care about breaking the
experimental ones.

Re: [PATCH net-next] net: ieee802154: Fix compilation error when CONFIG_IEEE802154_NL802154_EXPERIMENTAL is disabled

[Jakub Kicinski]

On Tue, 30 Aug 2022 23:31:24 -0700 Jakub Kicinski wrote:
> Hm, let me add 802154 folks.
> 
> Either we should treat the commands as reserved in terms of uAPI
> even if they get removed the IDs won't be reused, or they are for
> testing purposes only.
> 
> In the former case we should just remove the #ifdef around the values
> in the enum, it just leads to #ifdef proliferation while having no
> functional impact.
> 
> In the latter case we should start error checking from the last
> non-experimental command, as we don't care about breaking the
> experimental ones.

I haven't gone thru all of my inbox yet, but I see no reply from Stefan
or Alexander. My vote is to un-hide the EXPERIMENTAL commands.

Re: [PATCH net-next] net: ieee802154: Fix compilation error when CONFIG_IEEE802154_NL802154_EXPERIMENTAL is disabled

[Alexander Aring]

Hi,

On Wed, Aug 31, 2022 at 2:26 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 30 Aug 2022 23:31:24 -0700 Jakub Kicinski wrote:
> > Hm, let me add 802154 folks.
> >
> > Either we should treat the commands as reserved in terms of uAPI
> > even if they get removed the IDs won't be reused, or they are for
> > testing purposes only.
> >
> > In the former case we should just remove the #ifdef around the values
> > in the enum, it just leads to #ifdef proliferation while having no
> > functional impact.
> >
> > In the latter case we should start error checking from the last
> > non-experimental command, as we don't care about breaking the
> > experimental ones.
>
> I haven't gone thru all of my inbox yet, but I see no reply from Stefan
> or Alexander. My vote is to un-hide the EXPERIMENTAL commands.
>

fine for me if it's still in nl802154 and ends in error if somebody
tries to use it and it's not compiled. But users should still consider
it's not a stable API yet and we don't care about breaking it for
now...

- Alex

Re: [PATCH net-next] net: ieee802154: Fix compilation error when CONFIG_IEEE802154_NL802154_EXPERIMENTAL is disabled

[Stefan Schmidt]

Hello Jakub.

On 31.08.22 20:21, Jakub Kicinski wrote:
> On Tue, 30 Aug 2022 23:31:24 -0700 Jakub Kicinski wrote:
>> Hm, let me add 802154 folks.
>>
>> Either we should treat the commands as reserved in terms of uAPI
>> even if they get removed the IDs won't be reused, or they are for
>> testing purposes only.
>>
>> In the former case we should just remove the #ifdef around the values
>> in the enum, it just leads to #ifdef proliferation while having no
>> functional impact.
>>
>> In the latter case we should start error checking from the last
>> non-experimental command, as we don't care about breaking the
>> experimental ones.
> 
> I haven't gone thru all of my inbox yet, but I see no reply from Stefan
> or Alexander. My vote is to un-hide the EXPERIMENTAL commands.

I was swamped today and I am only now finding time to go through mail.

Given the problem these ifdef are raising I am ok with having these 
commands exposed without them.

Our main reason for having this feature marked as experimental is that 
it does not have much exposure and we fear that some of it needs rewrites.

If that really is going to happen we will simply treat the current 
commands as reserved/burned and come up with other ones if needed. While 
I hope this will not be needed it is a fair plan for mitigating this.

regards
Stefan Schmidt

Re: [PATCH net-next] net: ieee802154: Fix compilation error when CONFIG_IEEE802154_NL802154_EXPERIMENTAL is disabled

[Jakub Kicinski]

On Wed, 31 Aug 2022 22:59:14 +0200 Stefan Schmidt wrote:
> I was swamped today and I am only now finding time to go through mail.
> 
> Given the problem these ifdef are raising I am ok with having these 
> commands exposed without them.
> 
> Our main reason for having this feature marked as experimental is that 
> it does not have much exposure and we fear that some of it needs rewrites.
> 
> If that really is going to happen we will simply treat the current 
> commands as reserved/burned and come up with other ones if needed. While 
> I hope this will not be needed it is a fair plan for mitigating this.

Thanks for the replies. I keep going back and forth in my head on
what's better - un-hiding or just using NL802154_CMD_SET_WPAN_PHY_NETNS + 1 
as the start of validation, since it's okay to break experimental commands.

Any preference?

Re: [PATCH net-next] net: ieee802154: Fix compilation error when CONFIG_IEEE802154_NL802154_EXPERIMENTAL is disabled

[Stefan Schmidt]

Hello Jakub.

On 31.08.22 23:09, Jakub Kicinski wrote:
> On Wed, 31 Aug 2022 22:59:14 +0200 Stefan Schmidt wrote:
>> I was swamped today and I am only now finding time to go through mail.
>>
>> Given the problem these ifdef are raising I am ok with having these
>> commands exposed without them.
>>
>> Our main reason for having this feature marked as experimental is that
>> it does not have much exposure and we fear that some of it needs rewrites.
>>
>> If that really is going to happen we will simply treat the current
>> commands as reserved/burned and come up with other ones if needed. While
>> I hope this will not be needed it is a fair plan for mitigating this.
> 
> Thanks for the replies. I keep going back and forth in my head on
> what's better - un-hiding or just using NL802154_CMD_SET_WPAN_PHY_NETNS + 1
> as the start of validation, since it's okay to break experimental commands.
> 
> Any preference?

We saw other problems with these being behind ifdefs from build and 
fuzzing bots. I say its time we un-hide and deal with them being 
formerly deprecated and replaced by something else if it really comes to 
changes for them (which we are not sure of)

regards
Stefan Schmidt

Re: [PATCH net-next] net: ieee802154: Fix compilation error when CONFIG_IEEE802154_NL802154_EXPERIMENTAL is disabled

[Leon Romanovsky]

On Wed, Aug 31, 2022 at 02:09:47PM -0700, Jakub Kicinski wrote:
> On Wed, 31 Aug 2022 22:59:14 +0200 Stefan Schmidt wrote:
> > I was swamped today and I am only now finding time to go through mail.
> > 
> > Given the problem these ifdef are raising I am ok with having these 
> > commands exposed without them.
> > 
> > Our main reason for having this feature marked as experimental is that 
> > it does not have much exposure and we fear that some of it needs rewrites.
> > 
> > If that really is going to happen we will simply treat the current 
> > commands as reserved/burned and come up with other ones if needed. While 
> > I hope this will not be needed it is a fair plan for mitigating this.
> 
> Thanks for the replies. I keep going back and forth in my head on
> what's better - un-hiding or just using NL802154_CMD_SET_WPAN_PHY_NETNS + 1 
> as the start of validation, since it's okay to break experimental commands.
> 
> Any preference?

Jakub,

There is no such thing like experimental UAPI. Once you put something
in UAPI headers and/or allowed users to issue calls from userspace
to kernel, they can use it. We don't control how users compile their
kernels.

So it is not break "experimental commands", but break commands that
maybe shouldn't exist in first place.

nl802154 code suffers from two basic mistakes:
1. User visible defines are not part of UAPI headers. For example,
include/net/nl802154.h should be in include/uapi/net/....
2. Used Kconfig option for pseudo-UAPI header.

In this specific case, I checked that Fedora didn't enable this
CONFIG_IEEE802154_NL802154_EXPERIMENTAL knob, but someone needs
to check debian and other distros too.

Most likely it is not used at all.

https://src.fedoraproject.org/rpms/kernel/tree/rawhide

Thanks

Re: [PATCH net-next] net: ieee802154: Fix compilation error when CONFIG_IEEE802154_NL802154_EXPERIMENTAL is disabled

[Alexander Aring]

Hi,

On Thu, Sep 1, 2022 at 2:38 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Wed, Aug 31, 2022 at 02:09:47PM -0700, Jakub Kicinski wrote:
> > On Wed, 31 Aug 2022 22:59:14 +0200 Stefan Schmidt wrote:
> > > I was swamped today and I am only now finding time to go through mail.
> > >
> > > Given the problem these ifdef are raising I am ok with having these
> > > commands exposed without them.
> > >
> > > Our main reason for having this feature marked as experimental is that
> > > it does not have much exposure and we fear that some of it needs rewrites.
> > >
> > > If that really is going to happen we will simply treat the current
> > > commands as reserved/burned and come up with other ones if needed. While
> > > I hope this will not be needed it is a fair plan for mitigating this.
> >
> > Thanks for the replies. I keep going back and forth in my head on
> > what's better - un-hiding or just using NL802154_CMD_SET_WPAN_PHY_NETNS + 1
> > as the start of validation, since it's okay to break experimental commands.
> >
> > Any preference?
>
> Jakub,
>
> There is no such thing like experimental UAPI. Once you put something
> in UAPI headers and/or allowed users to issue calls from userspace
> to kernel, they can use it. We don't control how users compile their
> kernels.
>
> So it is not break "experimental commands", but break commands that
> maybe shouldn't exist in first place.
>
> nl802154 code suffers from two basic mistakes:
> 1. User visible defines are not part of UAPI headers. For example,
> include/net/nl802154.h should be in include/uapi/net/....

yes, but no because then this will end in breaking UAPI because it
will be exported to uapi headers if we change them?
For now we say everybody needs to copy the header on their own into
their user space application if they want to use the API which means
it fits for the kernel that they copied from.

> 2. Used Kconfig option for pseudo-UAPI header.
>
> In this specific case, I checked that Fedora didn't enable this
> CONFIG_IEEE802154_NL802154_EXPERIMENTAL knob, but someone needs
> to check debian and other distros too.
>

I would remove the CONFIG_IEEE802154_NL802154_EXPERIMENTAL config
option but don't move the header to "include/uapi/..." which means
that the whole nl802154 UAPI (and some others UAPIs) are still
experimental because it's not part of UAPI "directory".
btw: the whole subsystem is still experimental because f4671a90c418
("net/ieee802154: remove depends on CONFIG_EXPERIMENTAL") was never
acked by any maintainer... but indeed has other reasons why it got
removed.

- Alex

Re: [PATCH net-next] net: ieee802154: Fix compilation error when CONFIG_IEEE802154_NL802154_EXPERIMENTAL is disabled

[Leon Romanovsky]

On Thu, Sep 01, 2022 at 08:50:16AM -0400, Alexander Aring wrote:
> Hi,
> 
> On Thu, Sep 1, 2022 at 2:38 AM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Wed, Aug 31, 2022 at 02:09:47PM -0700, Jakub Kicinski wrote:
> > > On Wed, 31 Aug 2022 22:59:14 +0200 Stefan Schmidt wrote:
> > > > I was swamped today and I am only now finding time to go through mail.
> > > >
> > > > Given the problem these ifdef are raising I am ok with having these
> > > > commands exposed without them.
> > > >
> > > > Our main reason for having this feature marked as experimental is that
> > > > it does not have much exposure and we fear that some of it needs rewrites.
> > > >
> > > > If that really is going to happen we will simply treat the current
> > > > commands as reserved/burned and come up with other ones if needed. While
> > > > I hope this will not be needed it is a fair plan for mitigating this.
> > >
> > > Thanks for the replies. I keep going back and forth in my head on
> > > what's better - un-hiding or just using NL802154_CMD_SET_WPAN_PHY_NETNS + 1
> > > as the start of validation, since it's okay to break experimental commands.
> > >
> > > Any preference?
> >
> > Jakub,
> >
> > There is no such thing like experimental UAPI. Once you put something
> > in UAPI headers and/or allowed users to issue calls from userspace
> > to kernel, they can use it. We don't control how users compile their
> > kernels.
> >
> > So it is not break "experimental commands", but break commands that
> > maybe shouldn't exist in first place.
> >
> > nl802154 code suffers from two basic mistakes:
> > 1. User visible defines are not part of UAPI headers. For example,
> > include/net/nl802154.h should be in include/uapi/net/....
> 
> yes, but no because then this will end in breaking UAPI because it
> will be exported to uapi headers if we change them?
> For now we say everybody needs to copy the header on their own into
> their user space application if they want to use the API which means
> it fits for the kernel that they copied from.

It is not how UAPI works. Once you allowed me to send ID number XXX to
the kernel without any header file. I can use it directly, so "hiding"
files from users make their development harder, but not impossible.

Basically, this is how vendoring and fuzzing works.

> 
> > 2. Used Kconfig option for pseudo-UAPI header.
> >
> > In this specific case, I checked that Fedora didn't enable this
> > CONFIG_IEEE802154_NL802154_EXPERIMENTAL knob, but someone needs
> > to check debian and other distros too.
> >
> 
> I would remove the CONFIG_IEEE802154_NL802154_EXPERIMENTAL config
> option but don't move the header to "include/uapi/..." which means
> that the whole nl802154 UAPI (and some others UAPIs) are still
> experimental because it's not part of UAPI "directory".
> btw: the whole subsystem is still experimental because f4671a90c418
> ("net/ieee802154: remove depends on CONFIG_EXPERIMENTAL") was never
> acked by any maintainer... but indeed has other reasons why it got
> removed.

I don't know anything about NL802154, just trying to explain that UAPI
rules are both relevant to binary and compilation compatibility.

In your case, concept of CONFIG_IEEE802154_NL802154_EXPERIMENTAL breaks
binary compatibility.

Thanks

> 
> - Alex
> 

Re: [PATCH net-next] net: ieee802154: Fix compilation error when CONFIG_IEEE802154_NL802154_EXPERIMENTAL is disabled

[Jakub Kicinski]

On Thu, 1 Sep 2022 09:38:35 +0300 Leon Romanovsky wrote:
> There is no such thing like experimental UAPI. Once you put something
> in UAPI headers and/or allowed users to issue calls from userspace
> to kernel, they can use it. We don't control how users compile their
> kernels.
> 
> So it is not break "experimental commands", but break commands that
> maybe shouldn't exist in first place.
> 
> nl802154 code suffers from two basic mistakes:
> 1. User visible defines are not part of UAPI headers. For example,
> include/net/nl802154.h should be in include/uapi/net/....
> 2. Used Kconfig option for pseudo-UAPI header.
> 
> In this specific case, I checked that Fedora didn't enable this
> CONFIG_IEEE802154_NL802154_EXPERIMENTAL knob, but someone needs
> to check debian and other distros too.
> 
> Most likely it is not used at all.

You're right, FWIW. I didn't want to get sidetracked into that before
we fix the immediate build issue. It's not the only family playing uAPI
games :(

Re: [PATCH net-next] net: ieee802154: Fix compilation error when CONFIG_IEEE802154_NL802154_EXPERIMENTAL is disabled

[Alexander Aring]

Hi,

On Thu, Sep 1, 2022 at 4:23 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 1 Sep 2022 09:38:35 +0300 Leon Romanovsky wrote:
> > There is no such thing like experimental UAPI. Once you put something
> > in UAPI headers and/or allowed users to issue calls from userspace
> > to kernel, they can use it. We don't control how users compile their
> > kernels.
> >
> > So it is not break "experimental commands", but break commands that
> > maybe shouldn't exist in first place.
> >
> > nl802154 code suffers from two basic mistakes:
> > 1. User visible defines are not part of UAPI headers. For example,
> > include/net/nl802154.h should be in include/uapi/net/....
> > 2. Used Kconfig option for pseudo-UAPI header.
> >
> > In this specific case, I checked that Fedora didn't enable this
> > CONFIG_IEEE802154_NL802154_EXPERIMENTAL knob, but someone needs
> > to check debian and other distros too.
> >
> > Most likely it is not used at all.
>
> You're right, FWIW. I didn't want to get sidetracked into that before
> we fix the immediate build issue. It's not the only family playing uAPI
> games :(
>

I am not sure how to proceed here now, if removing the
CONFIG_IEEE802154_NL802154_EXPERIMENTAL option is the way to go. Then
do it?

It was a mistake to introduce that whole thing and a probably better
way is to change nl802154 is to mark it deprecated, after a while
rename the enum value to some reserved value and remove the associated
code. Then after some time it can be reused again? If this sounds like
a better idea how to handle the use case we have here?

I am sorry that this config currently causes such a big problem here.

- Alex

Re: [PATCH net-next] net: ieee802154: Fix compilation error when CONFIG_IEEE802154_NL802154_EXPERIMENTAL is disabled

[Jakub Kicinski]

On Thu, 1 Sep 2022 22:48:10 -0400 Alexander Aring wrote:
> > You're right, FWIW. I didn't want to get sidetracked into that before
> > we fix the immediate build issue. It's not the only family playing uAPI
> > games :(
> 
> I am not sure how to proceed here now, if removing the
> CONFIG_IEEE802154_NL802154_EXPERIMENTAL option is the way to go. Then
> do it?

Right, I was kinda expecting v2 from Gal but the weekend may have
started for him already, so let me send it myself.

Re: [PATCH net-next] net: ieee802154: Fix compilation error when CONFIG_IEEE802154_NL802154_EXPERIMENTAL is disabled

[Leon Romanovsky]

On Thu, Sep 01, 2022 at 08:00:12PM -0700, Jakub Kicinski wrote:
> On Thu, 1 Sep 2022 22:48:10 -0400 Alexander Aring wrote:
> > > You're right, FWIW. I didn't want to get sidetracked into that before
> > > we fix the immediate build issue. It's not the only family playing uAPI
> > > games :(
> > 
> > I am not sure how to proceed here now, if removing the
> > CONFIG_IEEE802154_NL802154_EXPERIMENTAL option is the way to go. Then
> > do it?
> 
> Right, I was kinda expecting v2 from Gal but the weekend may have
> started for him already, so let me send it myself.

We didn't know how v2 should look like and waited for some sort of
resolution.

Thanks