linux-kernel - Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

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]

Date:   Tue, 8 Aug 2023 15:47:59 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Marco Elver <elver@...gle.com>, linux-kernel@...r.kernel.org,
        kasan-dev@...glegroups.com, linux-mm@...ck.org,
        Steven Rostedt <rostedt@...dmis.org>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Alexander Potapenko <glider@...gle.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

On Tue, Aug 08, 2023 at 08:41:49AM +0200, Petr Mladek wrote:
> On Mon 2023-08-07 18:13:57, Andy Shevchenko wrote:
> > On Mon, Aug 07, 2023 at 06:11:24PM +0300, Andy Shevchenko wrote:
> > > On Mon, Aug 07, 2023 at 06:09:54PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Aug 07, 2023 at 05:03:19PM +0200, Petr Mladek wrote:
> > > > > On Sat 2023-08-05 20:50:26, Andy Shevchenko wrote:

...

> > > > > How does this sound, please?
> > > > 
> > > > Not every user (especially _header_) wants to have printk.h included just for
> > > > sprintf.h that may have nothing to do with real output. So, same reasoning
> > > > from me as keeping that in kernel.h, i.e. printk.h no better.
> > > 
> > > (haven't check these, just to show how many _headers_ uses sprintf() call)
> > > 
> > > $ git grep -lw s.*printf -- include/linux/
> > > include/linux/acpi.h
> > > include/linux/audit.h
> > > include/linux/btf.h
> > > include/linux/dev_printk.h
> > > include/linux/device-mapper.h
> > > include/linux/efi.h
> > > include/linux/fortify-string.h
> > > include/linux/fs.h
> > > include/linux/gameport.h
> > > include/linux/kdb.h
> > > include/linux/kdev_t.h
> > > include/linux/kernel.h
> > > include/linux/mmiotrace.h
> > > include/linux/netlink.h
> > > include/linux/pci-p2pdma.h
> > > include/linux/perf_event.h
> > > include/linux/printk.h
> > > include/linux/seq_buf.h
> > > include/linux/seq_file.h
> > > include/linux/shrinker.h
> > > include/linux/string.h
> > > include/linux/sunrpc/svc_xprt.h
> > > include/linux/tnum.h
> > > include/linux/trace_seq.h
> > > include/linux/usb.h
> > > include/linux/usb/gadget_configfs.h
> > 
> > Okay, revised as my regexp was too lazy
> > 
> > $ git grep -lw s[^[:space:]_]*printf -- include/linux/
> > include/linux/btf.h
> > include/linux/device-mapper.h
> > include/linux/efi.h
> > include/linux/fortify-string.h
> > include/linux/kdev_t.h
> > include/linux/kernel.h
> > include/linux/netlink.h
> > include/linux/pci-p2pdma.h
> > include/linux/perf_event.h
> > include/linux/sunrpc/svc_xprt.h
> > include/linux/tnum.h
> > include/linux/usb.h
> > include/linux/usb/gadget_configfs.h
> 
> This is only a tiny part of the picture.
> 
> $> git grep sc*n*printf | cut -d : -f1 | uniq | grep "\.c$" | wc -l
> 5254
> $> find . -name  "*.c" | wc -l
> 32319
> 
> It means that the vsprintf() family is used in 1/6 of all kernel
> source files. They would need to include one extra header.

No, not only one. more, but the outcome of this is not using what is not used
and unwinding the header dependency hell.

But hey, I am not talking about C files right now, it's secondary, however
in IIO we want to get rid of kernel.h in the C files as well.

Also, please, go through all of them and tell, how many of them are using
stuff from kernel.h besides sprintf.h and ARRAY_SIZE() (which I plan
for a long time to split from kernel.h)?

> If you split headers into so many small pieces then all
> source files will start with 3 screens of includes. I do not see
> how this helps with maintainability.

It should be a compromise. But including kernel.h mess into a file
(**especially** into header) for let's say a single sprintf() call
or use ARRAY_SIZE() macro is a bad idea. _This_ is not maintainable
code and developers definitely haven't put their brains to what they
are doing with the header inclusion block in their code.

-- 
With Best Regards,
Andy Shevchenko