[PATCH 1/5] x86/alternative: simplify DUMP_BYTES macro

[PATCH 1/5] x86/alternative: simplify DUMP_BYTES macro

[Alexey Dobriyan]

Avoid zero length check with clever whitespace placement in the format
string.

Signed-off-by: Alexey Dobriyan (CloudLinux) <adobriyan@gmail.com>
---
 arch/x86/kernel/alternative.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 5007c3ffe96f..6c9758ee6810 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -66,13 +66,10 @@ do {									\
 	if (unlikely(debug_alternative)) {				\
 		int j;							\
 									\
-		if (!(len))						\
-			break;						\
-									\
 		printk(KERN_DEBUG pr_fmt(fmt), ##args);			\
-		for (j = 0; j < (len) - 1; j++)				\
-			printk(KERN_CONT "%02hhx ", buf[j]);		\
-		printk(KERN_CONT "%02hhx\n", buf[j]);			\
+		for (j = 0; j < (len); j++)				\
+			printk(KERN_CONT " %02hhx", buf[j]);		\
+		printk(KERN_CONT "\n");					\
 	}								\
 } while (0)
 
@@ -214,7 +211,7 @@ static __always_inline int optimize_nops_range(u8 *instr, u8 instrlen, int off)
 	add_nops(instr + off, nnops);
 	local_irq_restore(flags);
 
-	DUMP_BYTES(instr, instrlen, "%px: [%d:%d) optimized NOPs: ", instr, off, i);
+	DUMP_BYTES(instr, instrlen, "%px: [%d:%d) optimized NOPs:", instr, off, i);
 
 	return nnops;
 }
@@ -303,8 +300,8 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 			instr, instr, a->instrlen,
 			replacement, a->replacementlen);
 
-		DUMP_BYTES(instr, a->instrlen, "%px:   old_insn: ", instr);
-		DUMP_BYTES(replacement, a->replacementlen, "%px:   rpl_insn: ", replacement);
+		DUMP_BYTES(instr, a->instrlen, "%px:   old_insn:", instr);
+		DUMP_BYTES(replacement, a->replacementlen, "%px:   rpl_insn:", replacement);
 
 		memcpy(insn_buff, replacement, a->replacementlen);
 		insn_buff_sz = a->replacementlen;
@@ -328,7 +325,7 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 		for (; insn_buff_sz < a->instrlen; insn_buff_sz++)
 			insn_buff[insn_buff_sz] = 0x90;
 
-		DUMP_BYTES(insn_buff, insn_buff_sz, "%px: final_insn: ", instr);
+		DUMP_BYTES(insn_buff, insn_buff_sz, "%px: final_insn:", instr);
 
 		text_poke_early(instr, insn_buff, insn_buff_sz);
 
@@ -499,8 +496,8 @@ void __init_or_module noinline apply_retpolines(s32 *start, s32 *end)
 		len = patch_retpoline(addr, &insn, bytes);
 		if (len == insn.length) {
 			optimize_nops(bytes, len);
-			DUMP_BYTES(((u8*)addr),  len, "%px: orig: ", addr);
-			DUMP_BYTES(((u8*)bytes), len, "%px: repl: ", addr);
+			DUMP_BYTES(((u8*)addr),  len, "%px: orig:", addr);
+			DUMP_BYTES(((u8*)bytes), len, "%px: repl:", addr);
 			text_poke_early(addr, bytes, len);
 		}
 	}
-- 
2.34.1

[PATCH 2/5] x86/alternative: bump MAX_PATCH_LEN

[Alexey Dobriyan]

Replacement instructions are raw data, there is no need to reserve
space for the NUL terminator.

Signed-off-by: Alexey Dobriyan (CloudLinux) <adobriyan@gmail.com>
---
 arch/x86/kernel/alternative.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 6c9758ee6810..1e6fd814dd08 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -35,7 +35,7 @@ int __read_mostly alternatives_patched;
 
 EXPORT_SYMBOL_GPL(alternatives_patched);
 
-#define MAX_PATCH_LEN (255-1)
+#define MAX_PATCH_LEN 255
 
 static int __initdata_or_module debug_alternative;
 
-- 
2.34.1

[PATCH 3/5] x86/alternative: record .altinstructions section entity size

[Alexey Dobriyan]

.altinstructions entry was 12 bytes in size, then it was 13 bytes,
now it is 12 again. It was 24 bytes on some distros as well.
Record this information as section sh_entsize value so that tools
which parse .altinstructions have easier time.

Signed-off-by: Alexey Dobriyan (CloudLinux) <adobriyan@gmail.com>
---
 arch/x86/include/asm/alternative.h | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h
index 58eee6402832..cf7722a106b3 100644
--- a/arch/x86/include/asm/alternative.h
+++ b/arch/x86/include/asm/alternative.h
@@ -9,6 +9,8 @@
 #define ALTINSTR_FLAG_INV	(1 << 15)
 #define ALT_NOT(feat)		((feat) | ALTINSTR_FLAG_INV)
 
+#define sizeof_struct_alt_instr 12
+
 #ifndef __ASSEMBLY__
 
 #include <linux/stddef.h>
@@ -66,6 +68,7 @@ struct alt_instr {
 	u8  instrlen;		/* length of original instruction */
 	u8  replacementlen;	/* length of new instruction */
 } __packed;
+_Static_assert(sizeof(struct alt_instr) == sizeof_struct_alt_instr, "");
 
 /*
  * Debug flag that can be tested to see whether alternative
@@ -159,7 +162,7 @@ static inline int alternatives_text_reserved(void *start, void *end)
 /* alternative assembly primitive: */
 #define ALTERNATIVE(oldinstr, newinstr, feature)			\
 	OLDINSTR(oldinstr, 1)						\
-	".pushsection .altinstructions,\"a\"\n"				\
+	".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
 	ALTINSTR_ENTRY(feature, 1)					\
 	".popsection\n"							\
 	".pushsection .altinstr_replacement, \"ax\"\n"			\
@@ -168,7 +171,7 @@ static inline int alternatives_text_reserved(void *start, void *end)
 
 #define ALTERNATIVE_2(oldinstr, newinstr1, feature1, newinstr2, feature2)\
 	OLDINSTR_2(oldinstr, 1, 2)					\
-	".pushsection .altinstructions,\"a\"\n"				\
+	".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
 	ALTINSTR_ENTRY(feature1, 1)					\
 	ALTINSTR_ENTRY(feature2, 2)					\
 	".popsection\n"							\
@@ -184,7 +187,7 @@ static inline int alternatives_text_reserved(void *start, void *end)
 
 #define ALTERNATIVE_3(oldinsn, newinsn1, feat1, newinsn2, feat2, newinsn3, feat3) \
 	OLDINSTR_3(oldinsn, 1, 2, 3)						\
-	".pushsection .altinstructions,\"a\"\n"					\
+	".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
 	ALTINSTR_ENTRY(feat1, 1)						\
 	ALTINSTR_ENTRY(feat2, 2)						\
 	ALTINSTR_ENTRY(feat3, 3)						\
@@ -331,7 +334,7 @@ static inline int alternatives_text_reserved(void *start, void *end)
 	.skip -(((144f-143f)-(141b-140b)) > 0) * ((144f-143f)-(141b-140b)),0x90
 142:
 
-	.pushsection .altinstructions,"a"
+	.pushsection .altinstructions,"aM",@progbits,sizeof_struct_alt_instr
 	altinstruction_entry 140b,143f,\feature,142b-140b,144f-143f
 	.popsection
 
@@ -368,7 +371,7 @@ static inline int alternatives_text_reserved(void *start, void *end)
 		(alt_max_short(new_len1, new_len2) - (old_len)),0x90
 142:
 
-	.pushsection .altinstructions,"a"
+	.pushsection .altinstructions,"aM",@progbits,sizeof_struct_alt_instr
 	altinstruction_entry 140b,143f,\feature1,142b-140b,144f-143f
 	altinstruction_entry 140b,144f,\feature2,142b-140b,145f-144f
 	.popsection
-- 
2.34.1

[PATCH 4/5] x86/alternative: make .altinstr_replacement section non-executable

[Alexey Dobriyan]

Instructions in .altinstr_replacement section aren't meant to be
executed by CPU directly, only after being copied to the real
instruction stream in .text/.init_text.

Signed-off-by: Alexey Dobriyan (CloudLinux) <adobriyan@gmail.com>
---
 arch/x86/include/asm/alternative.h | 10 +++++-----
 tools/objtool/check.c              |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h
index cf7722a106b3..d754a18edc99 100644
--- a/arch/x86/include/asm/alternative.h
+++ b/arch/x86/include/asm/alternative.h
@@ -165,7 +165,7 @@ static inline int alternatives_text_reserved(void *start, void *end)
 	".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
 	ALTINSTR_ENTRY(feature, 1)					\
 	".popsection\n"							\
-	".pushsection .altinstr_replacement, \"ax\"\n"			\
+	".pushsection .altinstr_replacement, \"a\"\n"			\
 	ALTINSTR_REPLACEMENT(newinstr, 1)				\
 	".popsection\n"
 
@@ -175,7 +175,7 @@ static inline int alternatives_text_reserved(void *start, void *end)
 	ALTINSTR_ENTRY(feature1, 1)					\
 	ALTINSTR_ENTRY(feature2, 2)					\
 	".popsection\n"							\
-	".pushsection .altinstr_replacement, \"ax\"\n"			\
+	".pushsection .altinstr_replacement, \"a\"\n"			\
 	ALTINSTR_REPLACEMENT(newinstr1, 1)				\
 	ALTINSTR_REPLACEMENT(newinstr2, 2)				\
 	".popsection\n"
@@ -192,7 +192,7 @@ static inline int alternatives_text_reserved(void *start, void *end)
 	ALTINSTR_ENTRY(feat2, 2)						\
 	ALTINSTR_ENTRY(feat3, 3)						\
 	".popsection\n"								\
-	".pushsection .altinstr_replacement, \"ax\"\n"				\
+	".pushsection .altinstr_replacement, \"a\"\n"				\
 	ALTINSTR_REPLACEMENT(newinsn1, 1)					\
 	ALTINSTR_REPLACEMENT(newinsn2, 2)					\
 	ALTINSTR_REPLACEMENT(newinsn3, 3)					\
@@ -338,7 +338,7 @@ static inline int alternatives_text_reserved(void *start, void *end)
 	altinstruction_entry 140b,143f,\feature,142b-140b,144f-143f
 	.popsection
 
-	.pushsection .altinstr_replacement,"ax"
+	.pushsection .altinstr_replacement,"a"
 143:
 	\newinstr
 144:
@@ -376,7 +376,7 @@ static inline int alternatives_text_reserved(void *start, void *end)
 	altinstruction_entry 140b,144f,\feature2,142b-140b,145f-144f
 	.popsection
 
-	.pushsection .altinstr_replacement,"ax"
+	.pushsection .altinstr_replacement,"a"
 143:
 	\newinstr1
 144:
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 7c33ec67c4a9..18c32035f41c 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -359,7 +359,7 @@ static int decode_instructions(struct objtool_file *file)
 
 	for_each_sec(file, sec) {
 
-		if (!(sec->sh.sh_flags & SHF_EXECINSTR))
+		if (!(sec->sh.sh_flags & SHF_EXECINSTR) && strcmp(sec->name, ".altinstr_replacement"))
 			continue;
 
 		if (strcmp(sec->name, ".altinstr_replacement") &&
-- 
2.34.1

[PATCH 5/5] x86/unwind/orc: delete dead write in __orc_find()

[Alexey Dobriyan]

Also move "mid" variable to the innermost scope and delete useless
parenthesis while I'm at it.

Signed-off-by: Alexey Dobriyan (CloudLinux) <adobriyan@gmail.com>
---
 arch/x86/kernel/unwind_orc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
index 2de3c8c5eba9..d38125ea1bf6 100644
--- a/arch/x86/kernel/unwind_orc.c
+++ b/arch/x86/kernel/unwind_orc.c
@@ -35,7 +35,7 @@ static struct orc_entry *__orc_find(int *ip_table, struct orc_entry *u_table,
 {
 	int *first = ip_table;
 	int *last = ip_table + num_entries - 1;
-	int *mid = first, *found = first;
+	int *found = first;
 
 	if (!num_entries)
 		return NULL;
@@ -47,7 +47,7 @@ static struct orc_entry *__orc_find(int *ip_table, struct orc_entry *u_table,
 	 * ignored when they conflict with a real entry.
 	 */
 	while (first <= last) {
-		mid = first + ((last - first) / 2);
+		int *mid = first + (last - first) / 2;
 
 		if (orc_ip(mid) <= ip) {
 			found = mid;
-- 
2.34.1

RE: [PATCH 5/5] x86/unwind/orc: delete dead write in __orc_find()

[David Laight]

From: Alexey Dobriyan
> Sent: 11 March 2022 14:43
> 
> Also move "mid" variable to the innermost scope and delete useless
> parenthesis while I'm at it.

Hiding the definition of 'mid' in the inner scope is pointless.

I'm also not 100% sure that 'found' is always set.
Consider the ip < orc_ip(first) case.

	David

> 
> Signed-off-by: Alexey Dobriyan (CloudLinux) <adobriyan@gmail.com>
> ---
>  arch/x86/kernel/unwind_orc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
> index 2de3c8c5eba9..d38125ea1bf6 100644
> --- a/arch/x86/kernel/unwind_orc.c
> +++ b/arch/x86/kernel/unwind_orc.c
> @@ -35,7 +35,7 @@ static struct orc_entry *__orc_find(int *ip_table, struct orc_entry *u_table,
>  {
>  	int *first = ip_table;
>  	int *last = ip_table + num_entries - 1;
> -	int *mid = first, *found = first;
> +	int *found = first;
> 
>  	if (!num_entries)
>  		return NULL;
> @@ -47,7 +47,7 @@ static struct orc_entry *__orc_find(int *ip_table, struct orc_entry *u_table,
>  	 * ignored when they conflict with a real entry.
>  	 */
>  	while (first <= last) {
> -		mid = first + ((last - first) / 2);
> +		int *mid = first + (last - first) / 2;
> 
>  		if (orc_ip(mid) <= ip) {
>  			found = mid;
> --
> 2.34.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 5/5] x86/unwind/orc: delete dead write in __orc_find()

[Alexey Dobriyan]

On Fri, Mar 11, 2022 at 03:13:02PM +0000, David Laight wrote:
> From: Alexey Dobriyan
> > Sent: 11 March 2022 14:43
> > 
> > Also move "mid" variable to the innermost scope and delete useless
> > parenthesis while I'm at it.
> 
> Hiding the definition of 'mid' in the inner scope is pointless.

Not, it is not.

> I'm also not 100% sure that 'found' is always set.

"found" is left as-is by the patch.

> Consider the ip < orc_ip(first) case.

> > --- a/arch/x86/kernel/unwind_orc.c
> > +++ b/arch/x86/kernel/unwind_orc.c
> > @@ -35,7 +35,7 @@ static struct orc_entry *__orc_find(int *ip_table, struct orc_entry *u_table,
> >  {
> >  	int *first = ip_table;
> >  	int *last = ip_table + num_entries - 1;
> > -	int *mid = first, *found = first;
> > +	int *found = first;
> > 
> >  	if (!num_entries)
> >  		return NULL;
> > @@ -47,7 +47,7 @@ static struct orc_entry *__orc_find(int *ip_table, struct orc_entry *u_table,
> >  	 * ignored when they conflict with a real entry.
> >  	 */
> >  	while (first <= last) {
> > -		mid = first + ((last - first) / 2);
> > +		int *mid = first + (last - first) / 2;
> > 
> >  		if (orc_ip(mid) <= ip) {
> >  			found = mid;

Re: [PATCH 4/5] x86/alternative: make .altinstr_replacement section non-executable

[Peter Zijlstra]

On Fri, Mar 11, 2022 at 05:43:11PM +0300, Alexey Dobriyan wrote:
> Instructions in .altinstr_replacement section aren't meant to be
> executed by CPU directly, only after being copied to the real
> instruction stream in .text/.init_text.

But who cares; the entire section is deleted right along with
.init_text.

Re: [PATCH 1/5] x86/alternative: simplify DUMP_BYTES macro

[Joe Perches]

On Fri, 2022-03-11 at 17:43 +0300, Alexey Dobriyan wrote:
> Avoid zero length check with clever whitespace placement in the format
> string.
[]
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
[]
> @@ -66,13 +66,10 @@ do {									\
>  	if (unlikely(debug_alternative)) {				\
>  		int j;							\
>  									\
> -		if (!(len))						\
> -			break;						\
> -									\
>  		printk(KERN_DEBUG pr_fmt(fmt), ##args);			\
> -		for (j = 0; j < (len) - 1; j++)				\
> -			printk(KERN_CONT "%02hhx ", buf[j]);		\
> -		printk(KERN_CONT "%02hhx\n", buf[j]);			\
> +		for (j = 0; j < (len); j++)				\
> +			printk(KERN_CONT " %02hhx", buf[j]);		\
> +		printk(KERN_CONT "\n");					\
>  	}								\

This could also use %02x and not %02hhx

And MAX_PATCH_LEN is 255 but is that really possible?

Maybe if the actual patch length is always <= 64 this could use
	printk(KERN_CONT "%*ph\n", (int)len, buf);
instead and avoid all possible interleaving?

If so, maybe just remove DUMP_BYTES and use DPRINTK directly.

Perhaps:
---
 arch/x86/kernel/alternative.c | 31 ++++++++++---------------------
 1 file changed, 10 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 018b61febf0e7..74fa946093467 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -61,21 +61,6 @@ do {									\
 		printk(KERN_DEBUG pr_fmt(fmt) "\n", ##args);		\
 } while (0)
 
-#define DUMP_BYTES(buf, len, fmt, args...)				\
-do {									\
-	if (unlikely(debug_alternative)) {				\
-		int j;							\
-									\
-		if (!(len))						\
-			break;						\
-									\
-		printk(KERN_DEBUG pr_fmt(fmt), ##args);			\
-		for (j = 0; j < (len) - 1; j++)				\
-			printk(KERN_CONT "%02hhx ", buf[j]);		\
-		printk(KERN_CONT "%02hhx\n", buf[j]);			\
-	}								\
-} while (0)
-
 static const unsigned char x86nops[] =
 {
 	BYTES_NOP1,
@@ -214,7 +199,8 @@ static __always_inline int optimize_nops_range(u8 *instr, u8 instrlen, int off)
 	add_nops(instr + off, nnops);
 	local_irq_restore(flags);
 
-	DUMP_BYTES(instr, instrlen, "%px: [%d:%d) optimized NOPs: ", instr, off, i);
+	DPRINTK("%px: [%d:%d) optimized NOPs: %*ph",
+		instr, off, i, (int)instrlen, instr);
 
 	return nnops;
 }
@@ -303,8 +289,10 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 			instr, instr, a->instrlen,
 			replacement, a->replacementlen);
 
-		DUMP_BYTES(instr, a->instrlen, "%px:   old_insn: ", instr);
-		DUMP_BYTES(replacement, a->replacementlen, "%px:   rpl_insn: ", replacement);
+		DPRINTK("%px:   old_insn: %*ph",
+			instr, (int)a->instrlen, instr);
+		DPRINTK("%px:   rpl_insn: %*ph",
+			replacement, (int)a->replacementlen, replacement);
 
 		memcpy(insn_buff, replacement, a->replacementlen);
 		insn_buff_sz = a->replacementlen;
@@ -328,7 +316,8 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 		for (; insn_buff_sz < a->instrlen; insn_buff_sz++)
 			insn_buff[insn_buff_sz] = 0x90;
 
-		DUMP_BYTES(insn_buff, insn_buff_sz, "%px: final_insn: ", instr);
+		DPRINTK("%px: final_insn: %*ph",
+			instr, (int)insn_buff_sz, insn_buff);
 
 		text_poke_early(instr, insn_buff, insn_buff_sz);
 
@@ -499,8 +488,8 @@ void __init_or_module noinline apply_retpolines(s32 *start, s32 *end)
 		len = patch_retpoline(addr, &insn, bytes);
 		if (len == insn.length) {
 			optimize_nops(bytes, len);
-			DUMP_BYTES(((u8*)addr),  len, "%px: orig: ", addr);
-			DUMP_BYTES(((u8*)bytes), len, "%px: repl: ", addr);
+			DPRINTK("%px: orig: %*ph", addr, (int)len, addr);
+			DPRINTK("%px: repl: %*ph", addr, (int)len, bytes);
 			text_poke_early(addr, bytes, len);
 		}
 	}

Re: [PATCH 3/5] x86/alternative: record .altinstructions section entity size

[Peter Zijlstra]

On Fri, Mar 11, 2022 at 05:43:10PM +0300, Alexey Dobriyan wrote:
> .altinstructions entry was 12 bytes in size, then it was 13 bytes,
> now it is 12 again. It was 24 bytes on some distros as well.
> Record this information as section sh_entsize value so that tools
> which parse .altinstructions have easier time.

Which tools would that be? Because afaict you've not actually updated
objtool.

> Signed-off-by: Alexey Dobriyan (CloudLinux) <adobriyan@gmail.com>
> ---
>  arch/x86/include/asm/alternative.h | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h
> index 58eee6402832..cf7722a106b3 100644
> --- a/arch/x86/include/asm/alternative.h
> +++ b/arch/x86/include/asm/alternative.h
> @@ -9,6 +9,8 @@
>  #define ALTINSTR_FLAG_INV	(1 << 15)
>  #define ALT_NOT(feat)		((feat) | ALTINSTR_FLAG_INV)
>  
> +#define sizeof_struct_alt_instr 12
> +
>  #ifndef __ASSEMBLY__
>  
>  #include <linux/stddef.h>
> @@ -66,6 +68,7 @@ struct alt_instr {
>  	u8  instrlen;		/* length of original instruction */
>  	u8  replacementlen;	/* length of new instruction */
>  } __packed;
> +_Static_assert(sizeof(struct alt_instr) == sizeof_struct_alt_instr, "");

Would it not be much simpler to have this in asm-offsets.h ?

> +	".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
> +	".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
> +	".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\

> +	.pushsection .altinstructions,"aM",@progbits,sizeof_struct_alt_instr
> +	.pushsection .altinstructions,"aM",@progbits,sizeof_struct_alt_instr

Aside of adding entsize, you're also adding the M(ergable) bit. Also,
those lines are on the unwieldy side of things.

Re: [PATCH 1/5] x86/alternative: simplify DUMP_BYTES macro

[Joe Perches]

On Sat, 2022-03-12 at 08:36 -0800, Joe Perches wrote:
> On Fri, 2022-03-11 at 17:43 +0300, Alexey Dobriyan wrote:
> > Avoid zero length check with clever whitespace placement in the format
> > string.
> []
> > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> []
> > @@ -66,13 +66,10 @@ do {									\
> >  	if (unlikely(debug_alternative)) {				\
> >  		int j;							\
> >  									\
> > -		if (!(len))						\
> > -			break;						\
> > -									\
> >  		printk(KERN_DEBUG pr_fmt(fmt), ##args);			\
> > -		for (j = 0; j < (len) - 1; j++)				\
> > -			printk(KERN_CONT "%02hhx ", buf[j]);		\
> > -		printk(KERN_CONT "%02hhx\n", buf[j]);			\
> > +		for (j = 0; j < (len); j++)				\
> > +			printk(KERN_CONT " %02hhx", buf[j]);		\
> > +		printk(KERN_CONT "\n");					\
> >  	}								\
> 
> This could also use %02x and not %02hhx
> 
> And MAX_PATCH_LEN is 255 but is that really possible?
> 
> Maybe if the actual patch length is always <= 64 this could use
> 	printk(KERN_CONT "%*ph\n", (int)len, buf);
> instead and avoid all possible interleaving?

Another possibility would be to raise the arbitrary 64 byte
limit on %*ph to 256.
---
 Documentation/core-api/printk-formats.rst | 6 +++---
 lib/vsprintf.c                            | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 5e89497ba314e..39f787e9b26e1 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -289,9 +289,9 @@ Raw buffer as a hex string
 	%*phD	00-01-02- ... -3f
 	%*phN	000102 ... 3f
 
-For printing small buffers (up to 64 bytes long) as a hex string with a
-certain separator. For larger buffers consider using
-:c:func:`print_hex_dump`.
+For printing small buffers (up to 256 bytes long) as a hex string with a
+certain separator. For buffers larger than 64 bytes consider using
+:c:func:`print_hex_dump` as its output can be more easily counted.
 
 MAC/FDDI addresses
 ------------------
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 2a6c767cc2709..be6fa9fab1be8 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1194,7 +1194,7 @@ char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 	}
 
 	if (spec.field_width > 0)
-		len = min_t(int, spec.field_width, 64);
+		len = min_t(int, spec.field_width, 256);
 
 	for (i = 0; i < len; ++i) {
 		if (buf < end)

Re: [PATCH 3/5] x86/alternative: record .altinstructions section entity size

[Alexey Dobriyan]

On Sat, Mar 12, 2022 at 10:17:40PM +0100, Peter Zijlstra wrote:
> On Fri, Mar 11, 2022 at 05:43:10PM +0300, Alexey Dobriyan wrote:
> > .altinstructions entry was 12 bytes in size, then it was 13 bytes,
> > now it is 12 again. It was 24 bytes on some distros as well.
> > Record this information as section sh_entsize value so that tools
> > which parse .altinstructions have easier time.
> 
> Which tools would that be? Because afaict you've not actually updated
> objtool.

We parse .altinstructions to look for "dangerous" functions so that we
don't unpatch when a process is sleeping in a userspace pagefault caused
by such function. Defining .sh_entsize will simplify this process in the future.
Now that padding issues have been solved, "struct alt_instr" should be
stable and sizeof should be enough to tell one layout from another.

> > --- a/arch/x86/include/asm/alternative.h
> > +++ b/arch/x86/include/asm/alternative.h
> > @@ -9,6 +9,8 @@
> >  #define ALTINSTR_FLAG_INV	(1 << 15)
> >  #define ALT_NOT(feat)		((feat) | ALTINSTR_FLAG_INV)
> >  
> > +#define sizeof_struct_alt_instr 12
> > +
> >  #ifndef __ASSEMBLY__
> >  
> >  #include <linux/stddef.h>
> > @@ -66,6 +68,7 @@ struct alt_instr {
> >  	u8  instrlen;		/* length of original instruction */
> >  	u8  replacementlen;	/* length of new instruction */
> >  } __packed;
> > +_Static_assert(sizeof(struct alt_instr) == sizeof_struct_alt_instr, "");
> 
> Would it not be much simpler to have this in asm-offsets.h ?

I tried this and failed. alternative.h is getting included and
preprocessed before asm-offsets.c is generated so there are lines like

	#define 12 12

and it doesn't work.

> > +	".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
> > +	".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
> > +	".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
> 
> > +	.pushsection .altinstructions,"aM",@progbits,sizeof_struct_alt_instr
> > +	.pushsection .altinstructions,"aM",@progbits,sizeof_struct_alt_instr
> 
> Aside of adding entsize, you're also adding the M(ergable) bit. Also,
> those lines are on the unwieldy side of things.

binutils doc says

	https://sourceware.org/binutils/docs/as/Section.html

	If flags contains the M symbol then the type argument must be specified as well as an extra argument—entsize—like this:

	.section name , "flags"M, @type, entsize

	Sections with the M flag but not S flag must contain fixed size constants,
	each entsize octets long. Sections with both M and S must contain zero
	terminated strings where each character is entsize bytes long. The linker
	may remove duplicates within sections with the same name, same entity size
	and same flags. entsize must be an absolute expression. For sections with
	both M and S, a string which is a suffix of a larger string is considered
	a duplicate. Thus "def" will be merged with "abcdef"; A reference to the
	first "def" will be changed to a reference to "abcdef"+3.

"a"M doesn't work, but "aM" does.

I don't know if merging is the issue, it is not like alt replacements have names.

Re: [PATCH 1/5] x86/alternative: simplify DUMP_BYTES macro

[Alexey Dobriyan]

On Sat, Mar 12, 2022 at 08:36:11AM -0800, Joe Perches wrote:
> On Fri, 2022-03-11 at 17:43 +0300, Alexey Dobriyan wrote:
> > Avoid zero length check with clever whitespace placement in the format
> > string.
> []
> > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> []
> > @@ -66,13 +66,10 @@ do {									\
> >  	if (unlikely(debug_alternative)) {				\
> >  		int j;							\
> >  									\
> > -		if (!(len))						\
> > -			break;						\
> > -									\
> >  		printk(KERN_DEBUG pr_fmt(fmt), ##args);			\
> > -		for (j = 0; j < (len) - 1; j++)				\
> > -			printk(KERN_CONT "%02hhx ", buf[j]);		\
> > -		printk(KERN_CONT "%02hhx\n", buf[j]);			\
> > +		for (j = 0; j < (len); j++)				\
> > +			printk(KERN_CONT " %02hhx", buf[j]);		\
> > +		printk(KERN_CONT "\n");					\
> >  	}								\
> 
> This could also use %02x and not %02hhx

I doubt as there is funky stuff possible with 255 and such values.
Format specifiers aren't the purpose of the patch anyway.

> And MAX_PATCH_LEN is 255 but is that really possible?

Yes if you try hard enough.

> Maybe if the actual patch length is always <= 64 this could use
> 	printk(KERN_CONT "%*ph\n", (int)len, buf);
> instead and avoid all possible interleaving?

It is for debugging feature nobody uses (because it works).

Re: [PATCH 1/5] x86/alternative: simplify DUMP_BYTES macro

[Joe Perches]

On Sun, 2022-03-13 at 21:09 +0300, Alexey Dobriyan wrote:
> On Sat, Mar 12, 2022 at 08:36:11AM -0800, Joe Perches wrote:
> > On Fri, 2022-03-11 at 17:43 +0300, Alexey Dobriyan wrote:
> > > Avoid zero length check with clever whitespace placement in the format
> > > string.
> > []
> > > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> > []
> > > @@ -66,13 +66,10 @@ do {									\
> > >  	if (unlikely(debug_alternative)) {				\
> > >  		int j;							\
> > >  									\
> > > -		if (!(len))						\
> > > -			break;						\
> > > -									\
> > >  		printk(KERN_DEBUG pr_fmt(fmt), ##args);			\
> > > -		for (j = 0; j < (len) - 1; j++)				\
> > > -			printk(KERN_CONT "%02hhx ", buf[j]);		\
> > > -		printk(KERN_CONT "%02hhx\n", buf[j]);			\
> > > +		for (j = 0; j < (len); j++)				\
> > > +			printk(KERN_CONT " %02hhx", buf[j]);		\
> > > +		printk(KERN_CONT "\n");					\
> > >  	}								\
> > 
> > This could also use %02x and not %02hhx
> 
> I doubt as there is funky stuff possible with 255 and such values.

'"%02hhx", u8' and '"%02x", u8' have the same output as the
u8 is converted anyway given the integer promotions.

https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q@mail.gmail.com/

> Format specifiers aren't the purpose of the patch anyway.

IMO: If you are already touching the lines,
     you might as well fix it at the same time.

cheers, Joe

Re: [PATCH 1/5] x86/alternative: simplify DUMP_BYTES macro

[Borislav Petkov]

On Fri, Mar 11, 2022 at 05:43:08PM +0300, Alexey Dobriyan wrote:
> Avoid zero length check with clever whitespace placement in the format
> string.
> 
> Signed-off-by: Alexey Dobriyan (CloudLinux) <adobriyan@gmail.com>
> ---
>  arch/x86/kernel/alternative.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 5007c3ffe96f..6c9758ee6810 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -66,13 +66,10 @@ do {									\
>  	if (unlikely(debug_alternative)) {				\
>  		int j;							\
>  									\
> -		if (!(len))						\
> -			break;						\
> -									\
>  		printk(KERN_DEBUG pr_fmt(fmt), ##args);			\
> -		for (j = 0; j < (len) - 1; j++)				\
> -			printk(KERN_CONT "%02hhx ", buf[j]);		\
> -		printk(KERN_CONT "%02hhx\n", buf[j]);			\
> +		for (j = 0; j < (len); j++)				\
> +			printk(KERN_CONT " %02hhx", buf[j]);		\
> +		printk(KERN_CONT "\n");					\
>  	}								\
>  } while (0)

That doesn't work always.

Before:

SMP alternatives: feat: 9*32+0, old: (current_save_fsgs+0x32/0xa0 (ffffffff81017762) len: 5), repl: (ffffffff89997c78, len: 0)
SMP alternatives: ffffffff81017762:   old_insn: eb 32 0f 1f 00
SMP alternatives: ffffffff81017762: final_insn: 90 90 90 90 90
SMP alternatives: ffffffff81017762: [0:5) optimized NOPs: 0f 1f 44 00 00


After:

SMP alternatives: feat: 9*32+0, old: (current_save_fsgs+0x32/0xa0 (ffffffff81017762) len: 5), repl: (ffffffff89997c78, len: 0)
SMP alternatives: ffffffff81017762:   old_insn: eb 32 0f 1f 00
SMP alternatives: ffffffff89997c78:   rpl_insn:			<----- *
SMP alternatives: ffffffff81017762: final_insn: 90 90 90 90 90
SMP alternatives: ffffffff81017762: [0:5) optimized NOPs: 0f 1f 44 00 00

there is no replacement insn in this case:

static __always_inline bool _static_cpu_has(u16 bit)
{
	asm_volatile_goto(
		ALTERNATIVE_TERNARY("jmp 6f", %P[feature], "", "jmp %l[t_no]")
						   	   ^^

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 1/5] x86/alternative: simplify DUMP_BYTES macro

[Thomas Gleixner]

On Fri, Mar 11 2022 at 17:43, Alexey Dobriyan wrote:
> Avoid zero length check with clever whitespace placement in the format
> string.
>
> Signed-off-by: Alexey Dobriyan (CloudLinux) <adobriyan@gmail.com>
> ---
>  arch/x86/kernel/alternative.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 5007c3ffe96f..6c9758ee6810 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -66,13 +66,10 @@ do {									\
>  	if (unlikely(debug_alternative)) {				\
>  		int j;							\
>  									\
> -		if (!(len))						\
> -			break;						\
> -									\

How does that clever whitespace placement prevent this being printed in
the len == 0 case, which is a legit case?

>  		printk(KERN_DEBUG pr_fmt(fmt), ##args);			\

This is debug muck. So why does it have to be "optimized"?

Thanks,

        tglx

Re: [PATCH 3/5] x86/alternative: record .altinstructions section entity size

[Thomas Gleixner]

On Sun, Mar 13 2022 at 21:05, Alexey Dobriyan wrote:
> On Sat, Mar 12, 2022 at 10:17:40PM +0100, Peter Zijlstra wrote:
>> On Fri, Mar 11, 2022 at 05:43:10PM +0300, Alexey Dobriyan wrote:
>> > +	".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
>> > +	".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
>> > +	".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
>> 
>> > +	.pushsection .altinstructions,"aM",@progbits,sizeof_struct_alt_instr
>> > +	.pushsection .altinstructions,"aM",@progbits,sizeof_struct_alt_instr
>> 
>> Aside of adding entsize, you're also adding the M(ergable) bit. Also,
>> those lines are on the unwieldy side of things.
>
> binutils doc says
>
> 	https://sourceware.org/binutils/docs/as/Section.html
>
> 	If flags contains the M symbol then the type argument must be specified as well as an extra argument—entsize—like this:
>
> 	.section name , "flags"M, @type, entsize
>
> 	Sections with the M flag but not S flag must contain fixed size constants,
> 	each entsize octets long. Sections with both M and S must contain zero
> 	terminated strings where each character is entsize bytes long. The linker
> 	may remove duplicates within sections with the same name, same entity size
> 	and same flags. entsize must be an absolute expression. For sections with
> 	both M and S, a string which is a suffix of a larger string is considered
> 	a duplicate. Thus "def" will be merged with "abcdef"; A reference to the
> 	first "def" will be changed to a reference to "abcdef"+3.
>
> "a"M doesn't work, but "aM" does.
>
> I don't know if merging is the issue, it is not like alt replacements have names.

That does not matter. M merges any duplications in sections with the
same [section] name, entity size and flags.

     .pushsection .bar "aM" @progbits, 4
     .byte 0x01, 0x02, 0x03, 0x04
     .popsection

     .pushsection .bar "aM" @progbits, 4
     .byte 0x01, 0x02, 0x03, 0x04
     .popsection

Will create a section .bar with lenght 4 and the content:
     0x1,0x2,0x3,0x4

What saves you here is the fact that the altinstruction entries are
guaranteed to be unique, but that wants a big fat comment.

Thanks,

        tglx

Re: [PATCH 3/5] x86/alternative: record .altinstructions section entity size

[Rasmus Villemoes]

On 05/04/2022 21.24, Thomas Gleixner wrote:
> On Sun, Mar 13 2022 at 21:05, Alexey Dobriyan wrote:

> That does not matter. M merges any duplications in sections with the
> same [section] name, entity size and flags.
> 
>      .pushsection .bar "aM" @progbits, 4
>      .byte 0x01, 0x02, 0x03, 0x04
>      .popsection
> 
>      .pushsection .bar "aM" @progbits, 4
>      .byte 0x01, 0x02, 0x03, 0x04
>      .popsection
> 
> Will create a section .bar with lenght 4 and the content:
>      0x1,0x2,0x3,0x4
> 
> What saves you here is the fact that the altinstruction entries are
> guaranteed to be unique, but that wants a big fat comment.

Actually, I think what saves this is that the linker at least currently
ignores the merge flag for sections with relocations; from binutils
bfd/merge.c:

  if ((sec->flags & SEC_RELOC) != 0)
    {
      /* We aren't prepared to handle relocations in merged sections.  */
      return true;
    }

I do think it is theoretically possible for two altinstruction entries
to end up being identical after relocations have been applied (same
relative offsets to both the .text section and their replacement
instructions).

Rasmus