| Message ID | 20201030181822.570402-1-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] Fonts: font_acorn_8x8: Replace discarded const qualifier | expand |
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: <stable@vger.kernel.org> > Cc: Russell King <linux@armlinux.org.uk> > Signed-off-by: Lee Jones <lee.jones@linaro.org> Tested-by: Peilin Ye <yepeilin.cs@gmail.com> Thank you, Peilin Ye
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.
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?
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.
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.
On Fri, Oct 30, 2020 at 7:18 PM Lee Jones <lee.jones@linaro.org> 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: <stable@vger.kernel.org> > Cc: Russell King <linux@armlinux.org.uk> > Signed-off-by: Lee Jones <lee.jones@linaro.org> 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 >
On Mon, 02 Nov 2020, Daniel Vetter wrote: > On Fri, Oct 30, 2020 at 7:18 PM Lee Jones <lee.jones@linaro.org> 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: <stable@vger.kernel.org> > > Cc: Russell King <linux@armlinux.org.uk> > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > 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.
On Mon, Nov 2, 2020 at 12:09 PM Lee Jones <lee.jones@linaro.org> wrote: > > On Mon, 02 Nov 2020, Daniel Vetter wrote: > > > On Fri, Oct 30, 2020 at 7:18 PM Lee Jones <lee.jones@linaro.org> 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: <stable@vger.kernel.org> > > > Cc: Russell King <linux@armlinux.org.uk> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > > > 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
On Mon, 02 Nov 2020, Daniel Vetter wrote: > On Mon, Nov 2, 2020 at 12:09 PM Lee Jones <lee.jones@linaro.org> wrote: > > > > On Mon, 02 Nov 2020, Daniel Vetter wrote: > > > > > On Fri, Oct 30, 2020 at 7:18 PM Lee Jones <lee.jones@linaro.org> 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: <stable@vger.kernel.org> > > > > Cc: Russell King <linux@armlinux.org.uk> > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > > > > > 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?
On Mon, Nov 2, 2020 at 12:30 PM Lee Jones <lee.jones@linaro.org> wrote: > > On Mon, 02 Nov 2020, Daniel Vetter wrote: > > > On Mon, Nov 2, 2020 at 12:09 PM Lee Jones <lee.jones@linaro.org> wrote: > > > > > > On Mon, 02 Nov 2020, Daniel Vetter wrote: > > > > > > > On Fri, Oct 30, 2020 at 7:18 PM Lee Jones <lee.jones@linaro.org> 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: <stable@vger.kernel.org> > > > > > Cc: Russell King <linux@armlinux.org.uk> > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > > > > > > > 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
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
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
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.
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.
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
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 */
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: <stable@vger.kernel.org> Cc: Russell King <linux@armlinux.org.uk> Signed-off-by: Lee Jones <lee.jones@linaro.org> --- lib/fonts/font_acorn_8x8.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)