[PATCH] mm/mremap: fix BUILD_BUG_ON() error in get_extent

[PATCH] mm/mremap: fix BUILD_BUG_ON() error in get_extent

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

clang cannt evaluate this function argument at compile time
when the function is not inlined, which leads to a link
time failure:

ld.lld: error: undefined symbol: __compiletime_assert_414
>>> referenced by mremap.c
>>>               mremap.o:(get_extent) in archive mm/built-in.a

Mark the function as __always_inline to avoid it.

Fixes: 9ad9718bfa41 ("mm/mremap: calculate extent in one place")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 mm/mremap.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index c5590afe7165..1cb464a07184 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -336,8 +336,9 @@ enum pgt_entry {
  * valid. Else returns a smaller extent bounded by the end of the source and
  * destination pgt_entry.
  */
-static unsigned long get_extent(enum pgt_entry entry, unsigned long old_addr,
-			unsigned long old_end, unsigned long new_addr)
+static __always_inline unsigned long get_extent(enum pgt_entry entry,
+			unsigned long old_addr, unsigned long old_end,
+			unsigned long new_addr)
 {
 	unsigned long next, extent, mask, size;
 
-- 
2.29.2

Re: [PATCH] mm/mremap: fix BUILD_BUG_ON() error in get_extent

[Vlastimil Babka]

The subject should say BUILD_BUG()

On 12/30/20 4:40 PM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> clang cannt evaluate this function argument at compile time
> when the function is not inlined, which leads to a link
> time failure:
> 
> ld.lld: error: undefined symbol: __compiletime_assert_414
>>>> referenced by mremap.c
>>>>               mremap.o:(get_extent) in archive mm/built-in.a
> 
> Mark the function as __always_inline to avoid it.

Not sure if it's the ideal fix, maybe just convert it to BUG() instead?
Functions shouldn't really have BUILD_BUG depending on parameters and rely on
inlining to make it work...

> Fixes: 9ad9718bfa41 ("mm/mremap: calculate extent in one place")

I think it's rather this one that introduces the BUILD_BUG() ?
c49dd3401802 ("mm: speedup mremap on 1GB or larger regions")

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  mm/mremap.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/mremap.c b/mm/mremap.c
> index c5590afe7165..1cb464a07184 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -336,8 +336,9 @@ enum pgt_entry {
>   * valid. Else returns a smaller extent bounded by the end of the source and
>   * destination pgt_entry.
>   */
> -static unsigned long get_extent(enum pgt_entry entry, unsigned long old_addr,
> -			unsigned long old_end, unsigned long new_addr)
> +static __always_inline unsigned long get_extent(enum pgt_entry entry,
> +			unsigned long old_addr, unsigned long old_end,
> +			unsigned long new_addr)
>  {
>  	unsigned long next, extent, mask, size;
>  
> 

Re: [PATCH] mm/mremap: fix BUILD_BUG_ON() error in get_extent

[Nathan Chancellor]

On Wed, Dec 30, 2020 at 04:40:40PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> clang cannt evaluate this function argument at compile time
> when the function is not inlined, which leads to a link
> time failure:
> 
> ld.lld: error: undefined symbol: __compiletime_assert_414
> >>> referenced by mremap.c
> >>>               mremap.o:(get_extent) in archive mm/built-in.a
> 
> Mark the function as __always_inline to avoid it.
> 
> Fixes: 9ad9718bfa41 ("mm/mremap: calculate extent in one place")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  mm/mremap.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/mremap.c b/mm/mremap.c
> index c5590afe7165..1cb464a07184 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -336,8 +336,9 @@ enum pgt_entry {
>   * valid. Else returns a smaller extent bounded by the end of the source and
>   * destination pgt_entry.
>   */
> -static unsigned long get_extent(enum pgt_entry entry, unsigned long old_addr,
> -			unsigned long old_end, unsigned long new_addr)
> +static __always_inline unsigned long get_extent(enum pgt_entry entry,
> +			unsigned long old_addr, unsigned long old_end,
> +			unsigned long new_addr)
>  {
>  	unsigned long next, extent, mask, size;
>  
> -- 
> 2.29.2
> 

I am in agreement with Vlastimil, I would rather see the BUILD_BUG()
dropped or converted into BUG() instead of papering over with
__always_inline. For what it's worth, I only see this build failure
with CONFIG_UBSAN_UNSIGNED_OVERFLOW, which you proposed disabling:

https://lore.kernel.org/lkml/20201230154749.746641-1-arnd@kernel.org/

Cheers,
Nathan

Re: [PATCH] mm/mremap: fix BUILD_BUG_ON() error in get_extent

[Arnd Bergmann]

On Mon, Jan 4, 2021 at 11:36 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> I am in agreement with Vlastimil, I would rather see the BUILD_BUG()
> dropped or converted into BUG() instead of papering over with
> __always_inline.

I see your point, but I also generally prefer build-time checks over
runtime ones wherever possible, and would prefer a way to keep
it in a form that allows that, at least if the check is considered useful
at all.

> For what it's worth, I only see this build failure
> with CONFIG_UBSAN_UNSIGNED_OVERFLOW, which you proposed disabling:
>
> https://lore.kernel.org/lkml/20201230154749.746641-1-arnd@kernel.org/

I'm building more randconfig kernels without this patch but with the
__always_inline
reverted now, will see if it comes back. If not, let's just drop this patch.

      Arnd

Re: [PATCH] mm/mremap: fix BUILD_BUG_ON() error in get_extent

[Nathan Chancellor]

On Wed, Dec 30, 2020 at 04:40:40PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> clang cannt evaluate this function argument at compile time
> when the function is not inlined, which leads to a link
> time failure:
> 
> ld.lld: error: undefined symbol: __compiletime_assert_414
> >>> referenced by mremap.c
> >>>               mremap.o:(get_extent) in archive mm/built-in.a
> 
> Mark the function as __always_inline to avoid it.
> 
> Fixes: 9ad9718bfa41 ("mm/mremap: calculate extent in one place")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

I would like to see some movement on getting this fixed in 5.11. As it
stands, this is one of three __compiletime_assert references with
CONFIG_UBSAN_UNSIGNED_OVERFLOW. If we want to keep the BUILD_BUG()
around, I think this is fine. Alternatively, turning it into a runtime
check would be fine too.

Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>

> ---
>  mm/mremap.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/mremap.c b/mm/mremap.c
> index c5590afe7165..1cb464a07184 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -336,8 +336,9 @@ enum pgt_entry {
>   * valid. Else returns a smaller extent bounded by the end of the source and
>   * destination pgt_entry.
>   */
> -static unsigned long get_extent(enum pgt_entry entry, unsigned long old_addr,
> -			unsigned long old_end, unsigned long new_addr)
> +static __always_inline unsigned long get_extent(enum pgt_entry entry,
> +			unsigned long old_addr, unsigned long old_end,
> +			unsigned long new_addr)
>  {
>  	unsigned long next, extent, mask, size;
>  
> -- 
> 2.29.2
 

Re: [PATCH] mm/mremap: fix BUILD_BUG_ON() error in get_extent

[Sedat Dilek]

On Tue, Jan 12, 2021 at 8:16 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Wed, Dec 30, 2020 at 04:40:40PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > clang cannt evaluate this function argument at compile time
> > when the function is not inlined, which leads to a link
> > time failure:
> >
> > ld.lld: error: undefined symbol: __compiletime_assert_414
> > >>> referenced by mremap.c
> > >>>               mremap.o:(get_extent) in archive mm/built-in.a
> >
> > Mark the function as __always_inline to avoid it.
> >
> > Fixes: 9ad9718bfa41 ("mm/mremap: calculate extent in one place")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> I would like to see some movement on getting this fixed in 5.11. As it
> stands, this is one of three __compiletime_assert references with
> CONFIG_UBSAN_UNSIGNED_OVERFLOW. If we want to keep the BUILD_BUG()
> around, I think this is fine. Alternatively, turning it into a runtime
> check would be fine too.
>
> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
>

I have this patch in my custom 5.11 queue.

Feel free to add my:

Tested-by: Sedat Dilek <sedat.dilek@gmail.com>

- Sedat -

> > ---
> >  mm/mremap.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/mremap.c b/mm/mremap.c
> > index c5590afe7165..1cb464a07184 100644
> > --- a/mm/mremap.c
> > +++ b/mm/mremap.c
> > @@ -336,8 +336,9 @@ enum pgt_entry {
> >   * valid. Else returns a smaller extent bounded by the end of the source and
> >   * destination pgt_entry.
> >   */
> > -static unsigned long get_extent(enum pgt_entry entry, unsigned long old_addr,
> > -                     unsigned long old_end, unsigned long new_addr)
> > +static __always_inline unsigned long get_extent(enum pgt_entry entry,
> > +                     unsigned long old_addr, unsigned long old_end,
> > +                     unsigned long new_addr)
> >  {
> >       unsigned long next, extent, mask, size;
> >
> > --
> > 2.29.2
>
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20210112191634.GA1587546%40ubuntu-m3-large-x86.

Re: [PATCH] mm/mremap: fix BUILD_BUG_ON() error in get_extent

[Nathan Chancellor]

On Tue, Jan 12, 2021 at 12:16:34PM -0700, Nathan Chancellor wrote:
> On Wed, Dec 30, 2020 at 04:40:40PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > clang cannt evaluate this function argument at compile time
> > when the function is not inlined, which leads to a link
> > time failure:
> > 
> > ld.lld: error: undefined symbol: __compiletime_assert_414
> > >>> referenced by mremap.c
> > >>>               mremap.o:(get_extent) in archive mm/built-in.a
> > 
> > Mark the function as __always_inline to avoid it.
> > 
> > Fixes: 9ad9718bfa41 ("mm/mremap: calculate extent in one place")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> I would like to see some movement on getting this fixed in 5.11. As it
> stands, this is one of three __compiletime_assert references with
> CONFIG_UBSAN_UNSIGNED_OVERFLOW. If we want to keep the BUILD_BUG()
> around, I think this is fine. Alternatively, turning it into a runtime
> check would be fine too.
> 
> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>

Ping? It is pretty late into the 5.11 cycle and this is still broken.

Cheers,
Nathan

> > ---
> >  mm/mremap.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/mm/mremap.c b/mm/mremap.c
> > index c5590afe7165..1cb464a07184 100644
> > --- a/mm/mremap.c
> > +++ b/mm/mremap.c
> > @@ -336,8 +336,9 @@ enum pgt_entry {
> >   * valid. Else returns a smaller extent bounded by the end of the source and
> >   * destination pgt_entry.
> >   */
> > -static unsigned long get_extent(enum pgt_entry entry, unsigned long old_addr,
> > -			unsigned long old_end, unsigned long new_addr)
> > +static __always_inline unsigned long get_extent(enum pgt_entry entry,
> > +			unsigned long old_addr, unsigned long old_end,
> > +			unsigned long new_addr)
> >  {
> >  	unsigned long next, extent, mask, size;
> >  
> > -- 
> > 2.29.2
>  

Re: [PATCH] mm/mremap: fix BUILD_BUG_ON() error in get_extent

[Kees Cook]

On Wed, Feb 03, 2021 at 11:48:40AM -0700, Nathan Chancellor wrote:
> On Tue, Jan 12, 2021 at 12:16:34PM -0700, Nathan Chancellor wrote:
> > On Wed, Dec 30, 2020 at 04:40:40PM +0100, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > > 
> > > clang cannt evaluate this function argument at compile time
> > > when the function is not inlined, which leads to a link
> > > time failure:
> > > 
> > > ld.lld: error: undefined symbol: __compiletime_assert_414
> > > >>> referenced by mremap.c
> > > >>>               mremap.o:(get_extent) in archive mm/built-in.a
> > > 
> > > Mark the function as __always_inline to avoid it.
> > > 
> > > Fixes: 9ad9718bfa41 ("mm/mremap: calculate extent in one place")
> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > 
> > I would like to see some movement on getting this fixed in 5.11. As it
> > stands, this is one of three __compiletime_assert references with
> > CONFIG_UBSAN_UNSIGNED_OVERFLOW. If we want to keep the BUILD_BUG()
> > around, I think this is fine. Alternatively, turning it into a runtime
> > check would be fine too.
> > 
> > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
> 
> Ping? It is pretty late into the 5.11 cycle and this is still broken.

I think we should just do the __always_inline. Who can take this?

-Kees

> 
> Cheers,
> Nathan
> 
> > > ---
> > >  mm/mremap.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/mm/mremap.c b/mm/mremap.c
> > > index c5590afe7165..1cb464a07184 100644
> > > --- a/mm/mremap.c
> > > +++ b/mm/mremap.c
> > > @@ -336,8 +336,9 @@ enum pgt_entry {
> > >   * valid. Else returns a smaller extent bounded by the end of the source and
> > >   * destination pgt_entry.
> > >   */
> > > -static unsigned long get_extent(enum pgt_entry entry, unsigned long old_addr,
> > > -			unsigned long old_end, unsigned long new_addr)
> > > +static __always_inline unsigned long get_extent(enum pgt_entry entry,
> > > +			unsigned long old_addr, unsigned long old_end,
> > > +			unsigned long new_addr)
> > >  {
> > >  	unsigned long next, extent, mask, size;
> > >  
> > > -- 
> > > 2.29.2
> >  

-- 
Kees Cook

Re: [PATCH] mm/mremap: fix BUILD_BUG_ON() error in get_extent

[Nathan Chancellor]

On Wed, Feb 03, 2021 at 12:03:07PM -0800, Kees Cook wrote:
> On Wed, Feb 03, 2021 at 11:48:40AM -0700, Nathan Chancellor wrote:
> > On Tue, Jan 12, 2021 at 12:16:34PM -0700, Nathan Chancellor wrote:
> > > On Wed, Dec 30, 2020 at 04:40:40PM +0100, Arnd Bergmann wrote:
> > > > From: Arnd Bergmann <arnd@arndb.de>
> > > > 
> > > > clang cannt evaluate this function argument at compile time
> > > > when the function is not inlined, which leads to a link
> > > > time failure:
> > > > 
> > > > ld.lld: error: undefined symbol: __compiletime_assert_414
> > > > >>> referenced by mremap.c
> > > > >>>               mremap.o:(get_extent) in archive mm/built-in.a
> > > > 
> > > > Mark the function as __always_inline to avoid it.
> > > > 
> > > > Fixes: 9ad9718bfa41 ("mm/mremap: calculate extent in one place")
> > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > > 
> > > I would like to see some movement on getting this fixed in 5.11. As it
> > > stands, this is one of three __compiletime_assert references with
> > > CONFIG_UBSAN_UNSIGNED_OVERFLOW. If we want to keep the BUILD_BUG()
> > > around, I think this is fine. Alternatively, turning it into a runtime
> > > check would be fine too.
> > > 
> > > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
> > 
> > Ping? It is pretty late into the 5.11 cycle and this is still broken.
> 
> I think we should just do the __always_inline. Who can take this?

This should probably go through -mm, unless we get an ack then Nick and
I could take it.

> > 
> > Cheers,
> > Nathan
> > 
> > > > ---
> > > >  mm/mremap.c | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/mm/mremap.c b/mm/mremap.c
> > > > index c5590afe7165..1cb464a07184 100644
> > > > --- a/mm/mremap.c
> > > > +++ b/mm/mremap.c
> > > > @@ -336,8 +336,9 @@ enum pgt_entry {
> > > >   * valid. Else returns a smaller extent bounded by the end of the source and
> > > >   * destination pgt_entry.
> > > >   */
> > > > -static unsigned long get_extent(enum pgt_entry entry, unsigned long old_addr,
> > > > -			unsigned long old_end, unsigned long new_addr)
> > > > +static __always_inline unsigned long get_extent(enum pgt_entry entry,
> > > > +			unsigned long old_addr, unsigned long old_end,
> > > > +			unsigned long new_addr)
> > > >  {
> > > >  	unsigned long next, extent, mask, size;
> > > >  
> > > > -- 
> > > > 2.29.2
> > >  
> 

Cheers,
Nathan

Re: [PATCH] mm/mremap: fix BUILD_BUG_ON() error in get_extent

[Andrew Morton]

On Fri, 5 Feb 2021 12:00:05 -0700 Nathan Chancellor <nathan@kernel.org> wrote:

> > > > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
> > > 
> > > Ping? It is pretty late into the 5.11 cycle and this is still broken.
> > 
> > I think we should just do the __always_inline. Who can take this?
> 
> This should probably go through -mm, unless we get an ack then Nick and
> I could take it.

I added it to -mm on Wednesday. 

Re: [PATCH] mm/mremap: fix BUILD_BUG_ON() error in get_extent

[Nathan Chancellor]

On Fri, Feb 05, 2021 at 01:02:23PM -0800, Andrew Morton wrote:
> On Fri, 5 Feb 2021 12:00:05 -0700 Nathan Chancellor <nathan@kernel.org> wrote:
> 
> > > > > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
> > > > 
> > > > Ping? It is pretty late into the 5.11 cycle and this is still broken.
> > > 
> > > I think we should just do the __always_inline. Who can take this?
> > 
> > This should probably go through -mm, unless we get an ack then Nick and
> > I could take it.
> 
> I added it to -mm on Wednesday. 

Thank you! I did not see an email but it looks like my tag did not make
it on to the patch so that would make sense. It would be great if this
could get into 5.11 but if not, we can always backport it

Cheers,
Nathan

Re: [PATCH] mm/mremap: fix BUILD_BUG_ON() error in get_extent

[Nick Desaulniers]

On Wed, Dec 30, 2020 at 7:41 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> clang cannt evaluate this function argument at compile time
> when the function is not inlined, which leads to a link
> time failure:
>
> ld.lld: error: undefined symbol: __compiletime_assert_414
> >>> referenced by mremap.c
> >>>               mremap.o:(get_extent) in archive mm/built-in.a
>
> Mark the function as __always_inline to avoid it.
>
> Fixes: 9ad9718bfa41 ("mm/mremap: calculate extent in one place")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Tested-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>  mm/mremap.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/mm/mremap.c b/mm/mremap.c
> index c5590afe7165..1cb464a07184 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -336,8 +336,9 @@ enum pgt_entry {
>   * valid. Else returns a smaller extent bounded by the end of the source and
>   * destination pgt_entry.
>   */
> -static unsigned long get_extent(enum pgt_entry entry, unsigned long old_addr,
> -                       unsigned long old_end, unsigned long new_addr)
> +static __always_inline unsigned long get_extent(enum pgt_entry entry,
> +                       unsigned long old_addr, unsigned long old_end,
> +                       unsigned long new_addr)
>  {
>         unsigned long next, extent, mask, size;
>
> --
> 2.29.2
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20201230154104.522605-1-arnd%40kernel.org.



-- 
Thanks,
~Nick Desaulniers