netdev - Re: [PATCH v2 net-next] net/hsr: add protocol version to fill

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c39e6626-02db-4a83-9f77-3d661f63ac0e@suse.de>
Date: Thu, 25 Sep 2025 10:37:38 +0200
From: Fernando Fernandez Mancera <fmancera@...e.de>
To: Jakub Kicinski <kuba@...nel.org>, Ján Václav
 <jvaclav@...hat.com>
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
 Simon Horman <horms@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH v2 net-next] net/hsr: add protocol version to fill_info
 output

On 9/25/25 1:40 AM, Jakub Kicinski wrote:
> On Wed, 24 Sep 2025 13:21:32 +0200 Ján Václav wrote:
>> On Wed, Sep 24, 2025 at 2:06 AM Jakub Kicinski <kuba@...nel.org> wrote:
>>> On Mon, 22 Sep 2025 11:37:45 +0200 Jan Vaclav wrote:
>>>>        if (hsr->prot_version == PRP_V1)
>>>>                proto = HSR_PROTOCOL_PRP;
>>>> +     if (nla_put_u8(skb, IFLA_HSR_VERSION, hsr->prot_version))
>>>> +             goto nla_put_failure;
>>>
>>> Looks like configuration path does not allow setting version if proto
>>> is PRP. Should we add an else before the if? since previous if is
>>> checking for PRP already
>>>   
>>
>> The way HSR configuration is currently handled seems very confusing to
>> me, because it allows setting the protocol version, but for PRP_V1
>> only as a byproduct of setting the protocol to PRP. If you configure
>> an interface with (proto = PRP, version = PRP_V1), it will fail, which
>> seems wrong to me, considering this is the end result of configuring
>> only with proto = PRP anyways.
> 
> I'm not very familiar with HSR or PRP. But The PRP_V1 which has value
> of 3 looks like a kernel-internal hack. Or does the protocol actually
> specify value 3 to mean PRP?
> 
> I don't think there's anything particularly wrong with the code.
> The version is for HSR because PRP only has one version, there's no
> ambiguity.
> 
> But again, I'm just glancing at the code I could be wrong..
> 

No you are right, this is a hack made to integrate PRP with HSR driver. 
PRP does not have a version other than PRP_V1 therefore it does not make 
much sense to configure it. Having said that, I think it's weird to 
report HSR_VERSION 3 but fail when configuring it.

IMHO HSR_VERSION should be hidden for PRP or it should be possible to 
configure it to "3" (which now that you say it, it looks weird).