netdev - Re: [GIT PULL] Networking for 6.0

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: <YusVM6B8bT6IOKdZ@salvia>
Date:   Thu, 4 Aug 2022 02:39:15 +0200
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Paolo Abeni <pabeni@...hat.com>, Vlad Buslov <vladbu@...dia.com>,
        Oz Shlomo <ozsh@...dia.com>, kuba@...nel.org,
        davem@...emloft.net, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [GIT PULL] Networking for 6.0

On Wed, Aug 03, 2022 at 05:27:07PM -0700, Linus Torvalds wrote:
> On Wed, Aug 3, 2022 at 5:11 PM Pablo Neira Ayuso <pablo@...filter.org> wrote:
> >
> > For these two questions, this new Kconfig toggle was copied from:
> >
> >  config NF_CONNTRACK_PROCFS
> >         bool "Supply CT list in procfs (OBSOLETE)"
> >         default y
> >         depends on PROC_FS
> >
> > which is under:
> >
> >  if NF_CONNTRACK
> >
> > but the copy and paste was missing this.
> 
> Note that there's two problems with that
> 
>  (1) the NF_CONNTRACK_PROCFS thing is 'default y' because it *USED* to
> be unconditional, and was split up as a config option back in 2011.
> 
> See commit 54b07dca6855 ("netfilter: provide config option to disable
> ancient procfs parts").
> 
> IOW, that NF_CONNTRACK_PROCFS exists and defaults to on, not because
> people added new code and wanted to make it default, but because the
> code used to always be enabled if NF_CONNTRACK was enabled, and people
> wanted the option to *disable* it.
> 
> That's when you do 'default y' - you take existing code that didn't
> originally have a question at all, and you make it optional. Then you
> use 'default y' so that people who used it don't get screwed in the
> process.
> 
>  (2) it didn't do the proper conditional on the feature it depended on.
> 
> So let's not do copy-and-paste programming. The old Kconfig snippet
> had completely different rules, had completely different history, and
> completely different default values as a result.
> 
> I realize that it's very easy to think of Kconfig as some
> not-very-important detail to just hook things up. But because it's
> front-facing to users, I do want people to think about it more than
> perhaps people otherwise would.

Agreed, it was a bad a idea to copy and paste it from
NF_CONNTRACK_PROCFS, this new toggle has nothing to do with it.

I'll take a closer look at any new Kconfig toggle coming in the
future to avoid issues like this.

Thanks for reviewing.