2020-10-30 18:20:30

by Lee Jones

[permalink] [raw]
Subject: [PATCH 1/1] Fonts: font_acorn_8x8: Replace discarded const qualifier

Commit 09e5b3fd5672 ("Fonts: Support FONT_EXTRA_WORDS macros for
built-in fonts") introduced the following error when building
rpc_defconfig (only this build appears to be affected):

`acorndata_8x8' referenced in section `.text' of arch/arm/boot/compressed/ll_char_wr.o:
defined in discarded section `.data' of arch/arm/boot/compressed/font.o
`acorndata_8x8' referenced in section `.data.rel.ro' of arch/arm/boot/compressed/font.o:
defined in discarded section `.data' of arch/arm/boot/compressed/font.o
make[3]: *** [/scratch/linux/arch/arm/boot/compressed/Makefile:191: arch/arm/boot/compressed/vmlinux] Error 1
make[2]: *** [/scratch/linux/arch/arm/boot/Makefile:61: arch/arm/boot/compressed/vmlinux] Error 2
make[1]: *** [/scratch/linux/arch/arm/Makefile:317: zImage] Error 2

The .data section is discarded at link time. Reinstating
acorndata_8x8 as const ensures it is still available after linking.

Cc: <[email protected]>
Cc: Russell King <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
---
lib/fonts/font_acorn_8x8.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/fonts/font_acorn_8x8.c b/lib/fonts/font_acorn_8x8.c
index 069b3e80c4344..fb395f0d40317 100644
--- a/lib/fonts/font_acorn_8x8.c
+++ b/lib/fonts/font_acorn_8x8.c
@@ -5,7 +5,7 @@

#define FONTDATAMAX 2048

-static struct font_data acorndata_8x8 = {
+static const struct font_data acorndata_8x8 = {
{ 0, 0, FONTDATAMAX, 0 }, {
/* 00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* ^@ */
/* 01 */ 0x7e, 0x81, 0xa5, 0x81, 0xbd, 0x99, 0x81, 0x7e, /* ^A */
--
2.25.1


2020-10-31 05:15:06

by Peilin Ye

[permalink] [raw]
Subject: Re: [PATCH 1/1] Fonts: font_acorn_8x8: Replace discarded const qualifier

Hi Lee,

On Fri, Oct 30, 2020 at 06:18:22PM +0000, Lee Jones wrote:
> Commit 09e5b3fd5672 ("Fonts: Support FONT_EXTRA_WORDS macros for
> built-in fonts") introduced the following error when building
> rpc_defconfig (only this build appears to be affected):
>
> `acorndata_8x8' referenced in section `.text' of arch/arm/boot/compressed/ll_char_wr.o:
> defined in discarded section `.data' of arch/arm/boot/compressed/font.o
> `acorndata_8x8' referenced in section `.data.rel.ro' of arch/arm/boot/compressed/font.o:
> defined in discarded section `.data' of arch/arm/boot/compressed/font.o
> make[3]: *** [/scratch/linux/arch/arm/boot/compressed/Makefile:191: arch/arm/boot/compressed/vmlinux] Error 1
> make[2]: *** [/scratch/linux/arch/arm/boot/Makefile:61: arch/arm/boot/compressed/vmlinux] Error 2
> make[1]: *** [/scratch/linux/arch/arm/Makefile:317: zImage] Error 2
>
> The .data section is discarded at link time. Reinstating
> acorndata_8x8 as const ensures it is still available after linking.

Thanks a lot for fixing this up! I wasn't aware that the symbol is being
referenced in arch/arm/boot/compressed/ll_char_wr.S. I'm sorry for the
trouble. The patch is,

> Cc: <[email protected]>
> Cc: Russell King <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>

Tested-by: Peilin Ye <[email protected]>

Thank you,
Peilin Ye

2020-10-31 10:28:54

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH 1/1] Fonts: font_acorn_8x8: Replace discarded const qualifier

On Fri, Oct 30, 2020 at 06:18:22PM +0000, Lee Jones wrote:
> Commit 09e5b3fd5672 ("Fonts: Support FONT_EXTRA_WORDS macros for

Your commit ID does not exist in mainline kernels, which makes this
confusing. The commit ID you should be using is 6735b4632def.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

2020-11-01 13:14:08

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH 1/1] Fonts: font_acorn_8x8: Replace discarded const qualifier

On Sat, 31 Oct 2020, Russell King - ARM Linux admin wrote:

> On Fri, Oct 30, 2020 at 06:18:22PM +0000, Lee Jones wrote:
> > Commit 09e5b3fd5672 ("Fonts: Support FONT_EXTRA_WORDS macros for
>
> Your commit ID does not exist in mainline kernels, which makes this
> confusing. The commit ID you should be using is 6735b4632def.

Ah yes, quite right. That is the ID from android-3.18 where this
issue was first seen and fixed against. I will fix it up for
Mainline.

Does the fix look okay to you though Russell?

--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

2020-11-02 10:26:30

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH 1/1] Fonts: font_acorn_8x8: Replace discarded const qualifier

On Sun, Nov 01, 2020 at 01:11:22PM +0000, Lee Jones wrote:
> On Sat, 31 Oct 2020, Russell King - ARM Linux admin wrote:
>
> > On Fri, Oct 30, 2020 at 06:18:22PM +0000, Lee Jones wrote:
> > > Commit 09e5b3fd5672 ("Fonts: Support FONT_EXTRA_WORDS macros for
> >
> > Your commit ID does not exist in mainline kernels, which makes this
> > confusing. The commit ID you should be using is 6735b4632def.
>
> Ah yes, quite right. That is the ID from android-3.18 where this
> issue was first seen and fixed against. I will fix it up for
> Mainline.
>
> Does the fix look okay to you though Russell?

Frankly, I don't know. Looking at the commit itself, it looks safe,
but it depends what this "extra" data is being used for. From what
I can see, the commit in question just adds the additional opaque
data as a member named "extra", and one is left to guess what it's
use as.

I'd have thought a small structure with named members would have
been the minimum given our standards for in-kernel code.

Why was the "const" dropped in the first place? Does this "extra"
member get written to somewhere?

So, sorry, no idea. This looks to me like a very unsatisfactory
commit, and probably something that got a very poor review.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

2020-11-02 10:32:29

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH 1/1] Fonts: font_acorn_8x8: Replace discarded const qualifier

On Mon, Nov 02, 2020 at 10:23:43AM +0000, Russell King - ARM Linux admin wrote:
> On Sun, Nov 01, 2020 at 01:11:22PM +0000, Lee Jones wrote:
> > On Sat, 31 Oct 2020, Russell King - ARM Linux admin wrote:
> >
> > > On Fri, Oct 30, 2020 at 06:18:22PM +0000, Lee Jones wrote:
> > > > Commit 09e5b3fd5672 ("Fonts: Support FONT_EXTRA_WORDS macros for
> > >
> > > Your commit ID does not exist in mainline kernels, which makes this
> > > confusing. The commit ID you should be using is 6735b4632def.
> >
> > Ah yes, quite right. That is the ID from android-3.18 where this
> > issue was first seen and fixed against. I will fix it up for
> > Mainline.
> >
> > Does the fix look okay to you though Russell?
>
> Frankly, I don't know. Looking at the commit itself, it looks safe,
> but it depends what this "extra" data is being used for. From what
> I can see, the commit in question just adds the additional opaque
> data as a member named "extra", and one is left to guess what it's
> use as.
>
> I'd have thought a small structure with named members would have
> been the minimum given our standards for in-kernel code.
>
> Why was the "const" dropped in the first place? Does this "extra"
> member get written to somewhere?
>
> So, sorry, no idea. This looks to me like a very unsatisfactory
> commit, and probably something that got a very poor review.

Also, the commit description is missing a chunk:

For user-provided fonts, the framebuffer layer resolves this issue by
reserving four extra words at the beginning of data buffers. Later,
whenever a function needs to access them, it simply uses the following
macros:

Recently we have gathered all the above macros to <linux/font.h>.

So what were these macros that have been nicely removed from the commit
description? I guess they started with a '#' character and git thought
they were a comment.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

2020-11-02 10:59:02

by Daniel Vetter

[permalink] [raw]
Subject: Re: [PATCH 1/1] Fonts: font_acorn_8x8: Replace discarded const qualifier

On Fri, Oct 30, 2020 at 7:18 PM Lee Jones <[email protected]> wrote:
>
> Commit 09e5b3fd5672 ("Fonts: Support FONT_EXTRA_WORDS macros for
> built-in fonts") introduced the following error when building
> rpc_defconfig (only this build appears to be affected):
>
> `acorndata_8x8' referenced in section `.text' of arch/arm/boot/compressed/ll_char_wr.o:
> defined in discarded section `.data' of arch/arm/boot/compressed/font.o
> `acorndata_8x8' referenced in section `.data.rel.ro' of arch/arm/boot/compressed/font.o:
> defined in discarded section `.data' of arch/arm/boot/compressed/font.o
> make[3]: *** [/scratch/linux/arch/arm/boot/compressed/Makefile:191: arch/arm/boot/compressed/vmlinux] Error 1
> make[2]: *** [/scratch/linux/arch/arm/boot/Makefile:61: arch/arm/boot/compressed/vmlinux] Error 2
> make[1]: *** [/scratch/linux/arch/arm/Makefile:317: zImage] Error 2
>
> The .data section is discarded at link time. Reinstating
> acorndata_8x8 as const ensures it is still available after linking.
>
> Cc: <[email protected]>
> Cc: Russell King <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>

Shouldn't we add the const to all of them, for consistency?
-Daniel

> ---
> lib/fonts/font_acorn_8x8.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/fonts/font_acorn_8x8.c b/lib/fonts/font_acorn_8x8.c
> index 069b3e80c4344..fb395f0d40317 100644
> --- a/lib/fonts/font_acorn_8x8.c
> +++ b/lib/fonts/font_acorn_8x8.c
> @@ -5,7 +5,7 @@
>
> #define FONTDATAMAX 2048
>
> -static struct font_data acorndata_8x8 = {
> +static const struct font_data acorndata_8x8 = {
> { 0, 0, FONTDATAMAX, 0 }, {
> /* 00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* ^@ */
> /* 01 */ 0x7e, 0x81, 0xa5, 0x81, 0xbd, 0x99, 0x81, 0x7e, /* ^A */
> --
> 2.25.1
>


--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2020-11-02 11:13:08

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH 1/1] Fonts: font_acorn_8x8: Replace discarded const qualifier

On Mon, 02 Nov 2020, Daniel Vetter wrote:

> On Fri, Oct 30, 2020 at 7:18 PM Lee Jones <[email protected]> wrote:
> >
> > Commit 09e5b3fd5672 ("Fonts: Support FONT_EXTRA_WORDS macros for
> > built-in fonts") introduced the following error when building
> > rpc_defconfig (only this build appears to be affected):
> >
> > `acorndata_8x8' referenced in section `.text' of arch/arm/boot/compressed/ll_char_wr.o:
> > defined in discarded section `.data' of arch/arm/boot/compressed/font.o
> > `acorndata_8x8' referenced in section `.data.rel.ro' of arch/arm/boot/compressed/font.o:
> > defined in discarded section `.data' of arch/arm/boot/compressed/font.o
> > make[3]: *** [/scratch/linux/arch/arm/boot/compressed/Makefile:191: arch/arm/boot/compressed/vmlinux] Error 1
> > make[2]: *** [/scratch/linux/arch/arm/boot/Makefile:61: arch/arm/boot/compressed/vmlinux] Error 2
> > make[1]: *** [/scratch/linux/arch/arm/Makefile:317: zImage] Error 2
> >
> > The .data section is discarded at link time. Reinstating
> > acorndata_8x8 as const ensures it is still available after linking.
> >
> > Cc: <[email protected]>
> > Cc: Russell King <[email protected]>
> > Signed-off-by: Lee Jones <[email protected]>
>
> Shouldn't we add the const to all of them, for consistency?

The thought did cross my mind. However, I do not see any further
issues which need addressing. Nor do I have any visibility into what
issues may be caused by doing so. The only thing I know for sure is
that this patch fixes the compile error pertained to in the commit
message, and I'd like for this fix to be as atomic as possible, as
it's designed to be routed through the Stable/LTS trees.

--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

2020-11-02 11:21:12

by Daniel Vetter

[permalink] [raw]
Subject: Re: [PATCH 1/1] Fonts: font_acorn_8x8: Replace discarded const qualifier

On Mon, Nov 2, 2020 at 12:09 PM Lee Jones <[email protected]> wrote:
>
> On Mon, 02 Nov 2020, Daniel Vetter wrote:
>
> > On Fri, Oct 30, 2020 at 7:18 PM Lee Jones <[email protected]> wrote:
> > >
> > > Commit 09e5b3fd5672 ("Fonts: Support FONT_EXTRA_WORDS macros for
> > > built-in fonts") introduced the following error when building
> > > rpc_defconfig (only this build appears to be affected):
> > >
> > > `acorndata_8x8' referenced in section `.text' of arch/arm/boot/compressed/ll_char_wr.o:
> > > defined in discarded section `.data' of arch/arm/boot/compressed/font.o
> > > `acorndata_8x8' referenced in section `.data.rel.ro' of arch/arm/boot/compressed/font.o:
> > > defined in discarded section `.data' of arch/arm/boot/compressed/font.o
> > > make[3]: *** [/scratch/linux/arch/arm/boot/compressed/Makefile:191: arch/arm/boot/compressed/vmlinux] Error 1
> > > make[2]: *** [/scratch/linux/arch/arm/boot/Makefile:61: arch/arm/boot/compressed/vmlinux] Error 2
> > > make[1]: *** [/scratch/linux/arch/arm/Makefile:317: zImage] Error 2
> > >
> > > The .data section is discarded at link time. Reinstating
> > > acorndata_8x8 as const ensures it is still available after linking.
> > >
> > > Cc: <[email protected]>
> > > Cc: Russell King <[email protected]>
> > > Signed-off-by: Lee Jones <[email protected]>
> >
> > Shouldn't we add the const to all of them, for consistency?
>
> The thought did cross my mind. However, I do not see any further
> issues which need addressing. Nor do I have any visibility into what
> issues may be caused by doing so. The only thing I know for sure is
> that this patch fixes the compile error pertained to in the commit
> message, and I'd like for this fix to be as atomic as possible, as
> it's designed to be routed through the Stable/LTS trees.

The trouble is that if we only make one of them const, then it'll take
so much longer to hit any issues due to code not handling this
correctly. Being consistent with all fonts sounds like the best
approach.

And the original patch that lost the const for the additional data
also went through cc: stable for all fonts together. So that shouldn't
be the hold-up.
-Daniel

>
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog



--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2020-11-02 11:32:37

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH 1/1] Fonts: font_acorn_8x8: Replace discarded const qualifier

On Mon, 02 Nov 2020, Daniel Vetter wrote:

> On Mon, Nov 2, 2020 at 12:09 PM Lee Jones <[email protected]> wrote:
> >
> > On Mon, 02 Nov 2020, Daniel Vetter wrote:
> >
> > > On Fri, Oct 30, 2020 at 7:18 PM Lee Jones <[email protected]> wrote:
> > > >
> > > > Commit 09e5b3fd5672 ("Fonts: Support FONT_EXTRA_WORDS macros for
> > > > built-in fonts") introduced the following error when building
> > > > rpc_defconfig (only this build appears to be affected):
> > > >
> > > > `acorndata_8x8' referenced in section `.text' of arch/arm/boot/compressed/ll_char_wr.o:
> > > > defined in discarded section `.data' of arch/arm/boot/compressed/font.o
> > > > `acorndata_8x8' referenced in section `.data.rel.ro' of arch/arm/boot/compressed/font.o:
> > > > defined in discarded section `.data' of arch/arm/boot/compressed/font.o
> > > > make[3]: *** [/scratch/linux/arch/arm/boot/compressed/Makefile:191: arch/arm/boot/compressed/vmlinux] Error 1
> > > > make[2]: *** [/scratch/linux/arch/arm/boot/Makefile:61: arch/arm/boot/compressed/vmlinux] Error 2
> > > > make[1]: *** [/scratch/linux/arch/arm/Makefile:317: zImage] Error 2
> > > >
> > > > The .data section is discarded at link time. Reinstating
> > > > acorndata_8x8 as const ensures it is still available after linking.
> > > >
> > > > Cc: <[email protected]>
> > > > Cc: Russell King <[email protected]>
> > > > Signed-off-by: Lee Jones <[email protected]>
> > >
> > > Shouldn't we add the const to all of them, for consistency?
> >
> > The thought did cross my mind. However, I do not see any further
> > issues which need addressing. Nor do I have any visibility into what
> > issues may be caused by doing so. The only thing I know for sure is
> > that this patch fixes the compile error pertained to in the commit
> > message, and I'd like for this fix to be as atomic as possible, as
> > it's designed to be routed through the Stable/LTS trees.
>
> The trouble is that if we only make one of them const, then it'll take
> so much longer to hit any issues due to code not handling this
> correctly. Being consistent with all fonts sounds like the best
> approach.
>
> And the original patch that lost the const for the additional data
> also went through cc: stable for all fonts together. So that shouldn't
> be the hold-up.

My plan was to keep the fix as simple as possible.

This is only an issue due to the odd handling of the compressed Arm
image which exclusively references 'acorndata_8x8' and discards it's
.data section.

I am happy to go with the majority on this though.

Does anyone else have an opinion?

--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

2020-11-02 14:52:52

by Daniel Vetter

[permalink] [raw]
Subject: Re: [PATCH 1/1] Fonts: font_acorn_8x8: Replace discarded const qualifier

On Mon, Nov 2, 2020 at 12:30 PM Lee Jones <[email protected]> wrote:
>
> On Mon, 02 Nov 2020, Daniel Vetter wrote:
>
> > On Mon, Nov 2, 2020 at 12:09 PM Lee Jones <[email protected]> wrote:
> > >
> > > On Mon, 02 Nov 2020, Daniel Vetter wrote:
> > >
> > > > On Fri, Oct 30, 2020 at 7:18 PM Lee Jones <[email protected]> wrote:
> > > > >
> > > > > Commit 09e5b3fd5672 ("Fonts: Support FONT_EXTRA_WORDS macros for
> > > > > built-in fonts") introduced the following error when building
> > > > > rpc_defconfig (only this build appears to be affected):
> > > > >
> > > > > `acorndata_8x8' referenced in section `.text' of arch/arm/boot/compressed/ll_char_wr.o:
> > > > > defined in discarded section `.data' of arch/arm/boot/compressed/font.o
> > > > > `acorndata_8x8' referenced in section `.data.rel.ro' of arch/arm/boot/compressed/font.o:
> > > > > defined in discarded section `.data' of arch/arm/boot/compressed/font.o
> > > > > make[3]: *** [/scratch/linux/arch/arm/boot/compressed/Makefile:191: arch/arm/boot/compressed/vmlinux] Error 1
> > > > > make[2]: *** [/scratch/linux/arch/arm/boot/Makefile:61: arch/arm/boot/compressed/vmlinux] Error 2
> > > > > make[1]: *** [/scratch/linux/arch/arm/Makefile:317: zImage] Error 2
> > > > >
> > > > > The .data section is discarded at link time. Reinstating
> > > > > acorndata_8x8 as const ensures it is still available after linking.
> > > > >
> > > > > Cc: <[email protected]>
> > > > > Cc: Russell King <[email protected]>
> > > > > Signed-off-by: Lee Jones <[email protected]>
> > > >
> > > > Shouldn't we add the const to all of them, for consistency?
> > >
> > > The thought did cross my mind. However, I do not see any further
> > > issues which need addressing. Nor do I have any visibility into what
> > > issues may be caused by doing so. The only thing I know for sure is
> > > that this patch fixes the compile error pertained to in the commit
> > > message, and I'd like for this fix to be as atomic as possible, as
> > > it's designed to be routed through the Stable/LTS trees.
> >
> > The trouble is that if we only make one of them const, then it'll take
> > so much longer to hit any issues due to code not handling this
> > correctly. Being consistent with all fonts sounds like the best
> > approach.
> >
> > And the original patch that lost the const for the additional data
> > also went through cc: stable for all fonts together. So that shouldn't
> > be the hold-up.
>
> My plan was to keep the fix as simple as possible.
>
> This is only an issue due to the odd handling of the compressed Arm
> image which exclusively references 'acorndata_8x8' and discards it's
> .data section.
>
> I am happy to go with the majority on this though.
>
> Does anyone else have an opinion?

Oh I don't want to hold up the fix, I'm just semi desperately looking
for people who care beyond "this is the most minimal thing for my use
case" since this entire area is orphaned. With the other things fixed
feel free to smash my ack onto this.

Maybe Peilin is going to include the full re-cosntification in a
cleanup series, dunno.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2020-11-02 16:16:05

by Peilin Ye

[permalink] [raw]
Subject: Re: [PATCH 1/1] Fonts: font_acorn_8x8: Replace discarded const qualifier

Hi Russell,

On Mon, Nov 02, 2020 at 10:23:43AM +0000, Russell King - ARM Linux admin wrote:
> On Sun, Nov 01, 2020 at 01:11:22PM +0000, Lee Jones wrote:
> > On Sat, 31 Oct 2020, Russell King - ARM Linux admin wrote:
> >
> > > On Fri, Oct 30, 2020 at 06:18:22PM +0000, Lee Jones wrote:
> > > > Commit 09e5b3fd5672 ("Fonts: Support FONT_EXTRA_WORDS macros for
> > >
> > > Your commit ID does not exist in mainline kernels, which makes this
> > > confusing. The commit ID you should be using is 6735b4632def.
> >
> > Ah yes, quite right. That is the ID from android-3.18 where this
> > issue was first seen and fixed against. I will fix it up for
> > Mainline.
> >
> > Does the fix look okay to you though Russell?
>
> Frankly, I don't know. Looking at the commit itself, it looks safe,
> but it depends what this "extra" data is being used for. From what
> I can see, the commit in question just adds the additional opaque
> data as a member named "extra", and one is left to guess what it's
> use as.

Thank you very much for looking into this. I apologize for the trouble
and confusion it has caused.

The motivation behind this commit, and commit 5af08640795b ("fbcon: Fix
global-out-of-bounds read in fbcon_get_font()") was to fix a decades-old
out-of-bounds access bug in the framebuffer layer.

However the framebuffer layer is doing bounds checking in a very strange
way, by hiding the buffer length before the buffer, then access it using
a negative-indexing macro:

#define FNTSIZE(fd) (((int *)(fd))[-2])

Other "extra" (so-called by the framebuffer layer) fields include:

#define REFCOUNT(fd) (((int *)(fd))[-1])

#define FNTCHARCNT(fd) (((int *)(fd))[-3])
#define FNTSUM(fd) (((int *)(fd))[-4])

...representing reference count, character count and checksum,
respectively.

The commit in question (6735b4632def) prepends the buffer length to each
of the built-in font buffers, so other functions in the framebuffer
layer can use FNTSIZE() on them. 5af08640795b uses it to fix that
out-of-bounds bug.

> I'd have thought a small structure with named members would have
> been the minimum given our standards for in-kernel code.

Yes, this is a temporary bug fix, and is far from satisfactory. We are
trying to replace these magic macros using a structure with properly
named members. It is taking more time than I imagined, but one day this
temporary fix will disappear from the kernel, I hope.

> Why was the "const" dropped in the first place? Does this "extra"
> member get written to somewhere?

No, I will try to come up with a solution without these fields being
writable.

> So, sorry, no idea. This looks to me like a very unsatisfactory
> commit, and probably something that got a very poor review.

I hope this helps explain it.

Again, I apologize for all the troubles. I will do more thorough testing
and practice writing a commit message. Thank you!

Sincerely,
Peilin Ye

2020-11-02 16:22:28

by Peilin Ye

[permalink] [raw]
Subject: Re: [PATCH 1/1] Fonts: font_acorn_8x8: Replace discarded const qualifier

On Mon, Nov 02, 2020 at 03:50:49PM +0100, Daniel Vetter wrote:
> Maybe Peilin is going to include the full re-cosntification in a
> cleanup series, dunno.

Sure, I will do it in a separate patch.

Thank you,
Peilin Ye

2020-11-02 16:26:48

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH 1/1] Fonts: font_acorn_8x8: Replace discarded const qualifier

On Mon, 02 Nov 2020, Peilin Ye wrote:

> On Mon, Nov 02, 2020 at 03:50:49PM +0100, Daniel Vetter wrote:
> > Maybe Peilin is going to include the full re-cosntification in a
> > cleanup series, dunno.
>
> Sure, I will do it in a separate patch.

Are you happy to conduct a proper clean-up on top of this patch?

This is currently broken in all of the LTS kernels, which I would like
to have fixed post-haste.

--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

2020-11-02 16:28:41

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH 1/1] Fonts: font_acorn_8x8: Replace discarded const qualifier

On Mon, 02 Nov 2020, Lee Jones wrote:

> On Mon, 02 Nov 2020, Peilin Ye wrote:
>
> > On Mon, Nov 02, 2020 at 03:50:49PM +0100, Daniel Vetter wrote:
> > > Maybe Peilin is going to include the full re-cosntification in a
> > > cleanup series, dunno.
> >
> > Sure, I will do it in a separate patch.
>
> Are you happy to conduct a proper clean-up on top of this patch?
>
> This is currently broken in all of the LTS kernels, which I would like
> to have fixed post-haste.

Of course, if it's *just* a matter of making all of the structures
const again, I will do that myself and post a fix either this evening
or tomorrow morning.

--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

2020-11-02 16:37:53

by Peilin Ye

[permalink] [raw]
Subject: Re: [PATCH 1/1] Fonts: font_acorn_8x8: Replace discarded const qualifier

On Mon, Nov 02, 2020 at 04:24:47PM +0000, Lee Jones wrote:
> On Mon, 02 Nov 2020, Peilin Ye wrote:
>
> > On Mon, Nov 02, 2020 at 03:50:49PM +0100, Daniel Vetter wrote:
> > > Maybe Peilin is going to include the full re-cosntification in a
> > > cleanup series, dunno.
> >
> > Sure, I will do it in a separate patch.
>
> Are you happy to conduct a proper clean-up on top of this patch?
>
> This is currently broken in all of the LTS kernels, which I would like
> to have fixed post-haste.

Sure I will do it now. Thank you,

Peilin Ye