linux-kernel - Re: [PATCH] mm: add prototype for __add_to_page_cache

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: <20201223011945.GO874@casper.infradead.org>
Date:   Wed, 23 Dec 2020 01:19:45 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Souptick Joarder <jrdr.linux@...il.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, Alex Shi <alex.shi@...ux.alibaba.com>
Subject: Re: [PATCH] mm: add prototype for __add_to_page_cache_locked()

On Tue, Dec 22, 2020 at 03:53:45PM -0800, Andrew Morton wrote:
> : A previous attempt to make this function static led to compilation
> : errors for a few architectures, because __add_to_page_cache_locked() is
> : referred to by BPF code.

Yes, but it's wrong, because it's not architecture dependent.  It
depends on CONFIG_DEBUG_INFO_BTF

> > > +/*
> > > + * Any attempt to mark this function as static leads to build failure
> > > + * for few architectures. Adding a prototype to silence gcc warning.
> > > + */
> > 
> > We don't need a comment here for this.  The commit log is enough.
> 
> I think it's OK - people do send patches which remove a prototype and
> also make the function static.  A tree-wide grep would catch the bpf
> reference but I suspect people tend to grep for "foo(" rather then
> "foo".

... and the same wrong information is present here.  If there's going to
be a comment here at least make it something informative like

/* Must be visible for error injection */

> > > +int __add_to_page_cache_locked(struct page *page, struct address_space *mapping,
> > > +		pgoff_t offset, gfp_t gfp, void **shadowp);
> > 
> > Please name that 'index', not 'offset'.
> 
> I too prefer index over offset.  
> 
> X1:/usr/src/linux-5.10> grep -r "pgoff_t offset" . | wc -l
> 52
> X1:/usr/src/linux-5.10> grep -r "pgoff_t index" . | wc -l 
> 250
> 
> But renaming this arg should be a separate patch.

... but this is a new prototype.  Prototype names don't have to match
the function name (and often don't ...)

> And I don't think we should be preparing large "rename offset to index"
> patches, please.  The value/noise ratio is too low.

I'm only fixing them as I change those functions.  I just object to
introducing new wrong ones.