netdev - Re: [PATCH v6 net-next 1/7] netlink: Add a macro to set policy message with format string

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: <pj41zlr0tdaq1w.fsf@u570694869fb251.ant.amazon.com>
Date:   Sat, 25 Mar 2023 16:49:34 +0300
From:   Shay Agroskin <shayagr@...zon.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     Paolo Abeni <pabeni@...hat.com>,
        David Miller <davem@...emloft.net>, <netdev@...r.kernel.org>,
        "Woodhouse, David" <dwmw@...zon.com>,
        "Machulsky, Zorik" <zorik@...zon.com>,
        "Matushevsky, Alexander" <matua@...zon.com>,
        "Saeed Bshara" <saeedb@...zon.com>,
        "Wilson, Matt" <msw@...zon.com>,
        "Liguori, Anthony" <aliguori@...zon.com>,
        "Bshara, Nafea" <nafea@...zon.com>,
        "Belgazal, Netanel" <netanel@...zon.com>,
        "Saidi, Ali" <alisaidi@...zon.com>,
        "Herrenschmidt, Benjamin" <benh@...zon.com>,
        "Kiyanovski, Arthur" <akiyano@...zon.com>,
        "Dagan, Noam" <ndagan@...zon.com>,
        "Arinzon, David" <darinzon@...zon.com>,
        "Itzko, Shahar" <itzko@...zon.com>,
        "Abboud, Osama" <osamaabb@...zon.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Andrew Lunn <andrew@...n.ch>,
        Guangbin Huang <huangguangbin2@...wei.com>,
        Jie Wang <wangjie125@...wei.com>,
        Johannes Berg <johannes@...solutions.net>,
        Edward Cree <ecree.xilinx@...il.com>,
        "Florian Westphal" <fw@...len.de>
Subject: Re: [PATCH v6 net-next 1/7] netlink: Add a macro to set policy message with
 format string


Jakub Kicinski <kuba@...nel.org> writes:

> CAUTION: This email originated from outside of the 
> organization. Do not click links or open attachments unless you 
> can confirm the sender and know the content is safe.
>
>
>
> On Thu, 23 Mar 2023 21:44:52 +0200 Shay Agroskin wrote:
>> > That's why we have the local variable called __extack, that 
>> > we
>> > *can*
>> > use multiple times, because it's a separate variable, 
>> > (extack)
>> > is
>> > evaluated only once, to initialize it...
>> >
>> > We don't need to copy the string formatting, unless I'm 
>> > missing
>> > something. Paolo was just asking for:
>>
>> There is an issue with shadowing __extack by NL_SET_ERR_MSG_FMT
>> causing the changes to __extack not to be propagated back to 
>> the
>> caller.
>> I'm not that big of an expert in C but changing __extack ->
>> _extack fixes the shadowing issue.
>>
>> Might not be the most robust solution, though it might suffice 
>> for
>> this use case.
>
> TBH the hierarchy should be the other way around, 
> NL_SET_ERR_MSG_FMT()
> should be converted to be:
>
> #define NL_SET_ERR_MSG_FMT(extack, attr, msg, args...) \
>         NL_SET_ERR_MSG_ATTR_POL_FMT(extack, NULL, NULL, msg, 
>         ##args)
>
> and that'd fix the shadowing, right?

Well ... It will but it will contradict the current order of calls 
as I see it.

NL_SET_ERR_MSG_FMT_MOD calls NL_SET_ERR_MSG_FMT which can be 
described as 'the former extends the latter'.

On the other hand NL_SET_ERR_MSG_ATTR_POL implements the message 
setting by itself and doesn't use NL_SET_ERR_MSG to set the 
message.

So it seems like we already have two approaches for macro ordering 
here and following your suggestion would create another type of 
call direction of 'the former shrinks the latter by setting to 
NULL its attributes'.
Overall given the nature of C macros and the weird issues arise by 
shadowing variables (ending up for some reason in not modifying 
the pointer at all..) I'd say I mostly prefer V7 version which 
re-implements the message setting and avoids creating such very 
hard to find issues later.

Then again I'd follow your implementation suggestion if you still 
prefer it (also I can modify the macro NL_SET_ERR_MSG to call 
NL_SET_ERR_MSG_ATTR_POL to be consistent with the other change)

Shay