Re: [PATCH v4] checkpatch: add check for snprintf to scnprintf
On Thu, Apr 11, 2024 at 1:56 PM Joe Perches wrote:
> It could.
>
> # {v}snprintf uses that should likely be {v}scnprintf
> if ($line =~ /\b((v?)snprintf)\s*\(/) {
> WARN("SNPRINTF",
> "Prefer ${2}scnprintf over $1 - see:
> https://github.com/KSPP/linux/issues/105\n; . $herecurr);
> }
>
>
>
> Though I also think it's better to use lore rather than github
I am fine with making the UX change in v5 regarding using ${2} and $1
but I wish someone could have said something about the Github links
earlier, we already have a pattern going with these string api
changes:
"Prefer strscpy over strcpy - see:
https://github.com/KSPP/linux/issues/88\n; . $herecurr);
}
...
# strlcpy uses that should likely be strscpy
if ($line =~ /\bstrlcpy\s*\(/) {
WARN("STRLCPY",
"Prefer strscpy over strlcpy - see:
https://github.com/KSPP/linux/issues/89\n; . $herecurr);
}
# strncpy uses that should likely be strscpy or strscpy_pad
if ($line =~ /\bstrncpy\s*\(/) {
WARN("STRNCPY",
"Prefer strscpy, strscpy_pad, or __nonstring over strncpy - see:
https://github.com/KSPP/linux/issues/90\n; . $herecurr);
}
# {v}snprintf uses that should likely be {v}scnprintf
if ($line =~ /\b(v|)snprintf\s*\(/) {
WARN("SNPRINTF",
"Prefer ${2}scnprintf over $1 - see:
https://github.com/KSPP/linux/issues/105\n; . $herecurr);
...
It should be noted that nowhere else is a lore link or github link
provided during a warning, so there really is no precedence. Joe what
should we do?
>
Re: [PATCH v4] checkpatch: add check for snprintf to scnprintf
Le 08/04/2024 à 22:53, Justin Stitt a écrit :
I am going to quote Lee Jones who has been doing some snprintf ->
scnprintf refactorings:
"There is a general misunderstanding amongst engineers that
{v}snprintf() returns the length of the data *actually* encoded into the
destination array. However, as per the C99 standard {v}snprintf()
really returns the length of the data that *would have been* written if
there were enough space for it. This misunderstanding has led to
buffer-overruns in the past. It's generally considered safer to use the
{v}scnprintf() variants in their place (or even sprintf() in simple
cases). So let's do that."
To help prevent new instances of snprintf() from popping up, let's add a
check to checkpatch.pl.
Suggested-by: Finn Thain
Signed-off-by: Justin Stitt
---
Changes in v4:
- also check for vsnprintf variant (thanks Bill)
- Link to v3:
https://lore.kernel.org/r/20240315-snprintf-checkpatch-v3-1-a451e7664...@google.com
Changes in v3:
- fix indentation
- add reference link (https://github.com/KSPP/linux/issues/105) (thanks Joe)
- Link to v2:
https://lore.kernel.org/r/20240221-snprintf-checkpatch-v2-1-9baeb59da...@google.com
Changes in v2:
- Had a vim moment and deleted a character before sending the patch.
- Replaced the character :)
- Link to v1:
https://lore.kernel.org/r/20240221-snprintf-checkpatch-v1-1-3ac5025b5...@google.com
---
From a discussion here [1].
[1]:
https://lore.kernel.org/all/0f9c95f9-2c14-eee6-7faf-635880edc...@linux-m68k.org/
---
scripts/checkpatch.pl | 6 ++
1 file changed, 6 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 9c4c4a61bc83..a0fd681ea837 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -7012,6 +7012,12 @@ sub process {
"Prefer strscpy, strscpy_pad, or __nonstring over
strncpy - see: https://github.com/KSPP/linux/issues/90\n; . $herecurr);
}
+# {v}snprintf uses that should likely be {v}scnprintf
+ if ($line =~ /\b(v|)snprintf\s*\(\s*/) {
Hi,
for my understanding, what is the purpose of the 2nd "\s*"?
IMHO, it could be just removed.
+ WARN("SNPRINTF",
+"Prefer {v}scnprintf over {v}snprintf - see:
https://github.com/KSPP/linux/issues/105\n; . $herecurr);
Maybe $1 instead of {v} in both places, so that is displays the real
function name that is and should be used?
CJ
+ }
+
# ethtool_sprintf uses that should likely be ethtool_puts
if ($line =~
/\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) {
if (WARN("PREFER_ETHTOOL_PUTS",
---
base-commit: b401b621758e46812da61fa58a67c3fd8d91de0d
change-id: 20240221-snprintf-checkpatch-a864ed67ebd0
Best regards,
--
Justin Stitt
Re: [PATCH v4] checkpatch: add check for snprintf to scnprintf
On Thu, 2024-04-11 at 22:01 +0200, Christophe JAILLET wrote:
> Le 08/04/2024 à 22:53, Justin Stitt a écrit :
> > I am going to quote Lee Jones who has been doing some snprintf ->
> > scnprintf refactorings:
> >
> > "There is a general misunderstanding amongst engineers that
> > {v}snprintf() returns the length of the data *actually* encoded into the
> > destination array. However, as per the C99 standard {v}snprintf()
> > really returns the length of the data that *would have been* written if
> > there were enough space for it. This misunderstanding has led to
> > buffer-overruns in the past. It's generally considered safer to use the
> > {v}scnprintf() variants in their place (or even sprintf() in simple
> > cases). So let's do that."
> >
> > To help prevent new instances of snprintf() from popping up, let's add a
> > check to checkpatch.pl.
> >
> > Suggested-by: Finn Thain
> > Signed-off-by: Justin Stitt
> > ---
> > Changes in v4:
> > - also check for vsnprintf variant (thanks Bill)
> > - Link to v3:
> > https://lore.kernel.org/r/20240315-snprintf-checkpatch-v3-1-a451e7664...@google.com
> >
> > Changes in v3:
> > - fix indentation
> > - add reference link (https://github.com/KSPP/linux/issues/105) (thanks Joe)
> > - Link to v2:
> > https://lore.kernel.org/r/20240221-snprintf-checkpatch-v2-1-9baeb59da...@google.com
> >
> > Changes in v2:
> > - Had a vim moment and deleted a character before sending the patch.
> > - Replaced the character :)
> > - Link to v1:
> > https://lore.kernel.org/r/20240221-snprintf-checkpatch-v1-1-3ac5025b5...@google.com
> > ---
> > From a discussion here [1].
> >
> > [1]:
> > https://lore.kernel.org/all/0f9c95f9-2c14-eee6-7faf-635880edc...@linux-m68k.org/
> > ---
> > scripts/checkpatch.pl | 6 ++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 9c4c4a61bc83..a0fd681ea837 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -7012,6 +7012,12 @@ sub process {
> > "Prefer strscpy, strscpy_pad, or __nonstring over
> > strncpy - see: https://github.com/KSPP/linux/issues/90\n; . $herecurr);
> > }
> >
> > +# {v}snprintf uses that should likely be {v}scnprintf
> > + if ($line =~ /\b(v|)snprintf\s*\(\s*/) {
>
> Hi,
>
> for my understanding, what is the purpose of the 2nd "\s*"?
> IMHO, it could be just removed.
It could.
# {v}snprintf uses that should likely be {v}scnprintf
if ($line =~ /\b((v?)snprintf)\s*\(/) {
WARN("SNPRINTF",
"Prefer ${2}scnprintf over $1 - see:
https://github.com/KSPP/linux/issues/105\n; . $herecurr);
}
Though I also think it's better to use lore rather than github
Re: [POC][RFC][PATCH 0/2] pstore/mm/x86: Add wildcard memmap to map pstore consistently
On Thu, 11 Apr 2024 16:11:55 -0300 "Guilherme G. Piccoli" wrote: > Thanks Steve! Like Kees, I've been wanting a consistent way of mapping > some RAM for pstore for a while, without resorting to platform drivers > like Chromebooks do... Great! > > The idea seems very interesting and helpful, I'll test it here. My only > concern / "complain" is that it's currently only implemented for builtin > ramoops, which is not the default in many distros (like Arch, Ubuntu, > Debian). I read patch 2 (and discussion), so I think would be good to > have that builtin helper implemented upfront to allow modular usage of > ramoops. What I think I could do is to have a check after memory is allocated to copy the table mapping (in the heap) if it is filled. The reason I did it this way was because it was the easiest way to save the label to address memory before memory is initialized. I use a __initdata array (why waste memory if it's hardly ever used). But, after memory is initialized, we can check if the table has content, and if so allocate a copy and store it there and use that table instead. That would give modules a way to find the address as well. -- Steve
Re: [POC][RFC][PATCH 0/2] pstore/mm/x86: Add wildcard memmap to map pstore consistently
On 09/04/2024 19:25, Luck, Tony wrote: >>> I forgot to mention that this makes it trivial for any machine that doesn't >>> clear memory on soft-reboot, to enable console ramoops (to have access to >>> the last boot dmesg without needing serial). >>> >>> I tested this on a couple of my test boxes and on QEMU, and it works rather >>> well. >> >> I've long wanted a "stable for this machine and kernel" memory region >> like this for pstore. It would make testing much easier. > > Which systems does this work on? I'd assume that servers (and anything > else with ECC memory) would nuke contents while resetting ECC to clean > state. > > -Tony Thanks Steve! Like Kees, I've been wanting a consistent way of mapping some RAM for pstore for a while, without resorting to platform drivers like Chromebooks do... The idea seems very interesting and helpful, I'll test it here. My only concern / "complain" is that it's currently only implemented for builtin ramoops, which is not the default in many distros (like Arch, Ubuntu, Debian). I read patch 2 (and discussion), so I think would be good to have that builtin helper implemented upfront to allow modular usage of ramoops. Now, responding to Tony: Steam Deck also uses pstore/ram to store logs, and I've tested in my AMD desktop, it does work. Seems disabling memory retraining in BIOS (to speedup boot?) is somewhat common, not sure for servers though. As Joel mentioned as well, quite common to use pstore/ram in ARM embedded world. Cheers, Guilherme
[PATCH 2/2] configs/hardening: Disable CONFIG_UBSAN_SIGNED_WRAP
kernel/configs/hardening.config turns on UBSAN for the bounds sanitizer,
as that in combination with trapping can stop the exploitation of buffer
overflows within the kernel. At the same time, hardening.config turns
off every other UBSAN sanitizer because trapping means all UBSAN reports
will be fatal and the problems brought up by other sanitizers generally
do not have security implications.
The signed integer overflow sanitizer was recently added back to the
kernel and it is default on with just CONFIG_UBSAN=y, meaning that it
gets enabled when merging hardening.config into another configuration.
While this sanitizer does have security implications like the array
bounds sanitizer, work to clean up enough instances to allow this to run
in production environments is still ramping up, which means regular
users and testers may be broken by these instances with
CONFIG_UBSAN_TRAP=y. Disable CONFIG_UBSAN_SIGNED_WRAP in
hardening.config to avoid this situation.
Fixes: 557f8c582a9b ("ubsan: Reintroduce signed overflow sanitizer")
Signed-off-by: Nathan Chancellor
---
kernel/configs/hardening.config | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/configs/hardening.config b/kernel/configs/hardening.config
index d6f6dc45628a..4b4cfcba3190 100644
--- a/kernel/configs/hardening.config
+++ b/kernel/configs/hardening.config
@@ -41,6 +41,7 @@ CONFIG_UBSAN_BOUNDS=y
# CONFIG_UBSAN_SHIFT is not set
# CONFIG_UBSAN_DIV_ZERO is not set
# CONFIG_UBSAN_UNREACHABLE is not set
+# CONFIG_UBSAN_SIGNED_WRAP is not set
# CONFIG_UBSAN_BOOL is not set
# CONFIG_UBSAN_ENUM is not set
# CONFIG_UBSAN_ALIGNMENT is not set
--
2.44.0
[PATCH 1/2] configs/hardening: Fix disabling UBSAN configurations
The initial change that added kernel/configs/hardening.config attempted
to disable all UBSAN sanitizers except for the array bounds one while
turning on UBSAN_TRAP. Unfortunately, it only got the syntax for
CONFIG_UBSAN_SHIFT correct, so configurations that are on by default
with CONFIG_UBSAN=y such as CONFIG_UBSAN_{BOOL,ENUM} do not get disabled
properly.
CONFIG_ARCH_HAS_UBSAN=y
CONFIG_UBSAN=y
CONFIG_UBSAN_TRAP=y
CONFIG_CC_HAS_UBSAN_BOUNDS_STRICT=y
CONFIG_UBSAN_BOUNDS=y
CONFIG_UBSAN_BOUNDS_STRICT=y
# CONFIG_UBSAN_SHIFT is not set
# CONFIG_UBSAN_DIV_ZERO is not set
# CONFIG_UBSAN_UNREACHABLE is not set
CONFIG_UBSAN_SIGNED_WRAP=y
CONFIG_UBSAN_BOOL=y
CONFIG_UBSAN_ENUM=y
# CONFIG_TEST_UBSAN is not set
Add the missing 'is not set' to each configuration that needs it so that
they get disabled as intended.
CONFIG_ARCH_HAS_UBSAN=y
CONFIG_UBSAN=y
CONFIG_UBSAN_TRAP=y
CONFIG_CC_HAS_UBSAN_BOUNDS_STRICT=y
CONFIG_UBSAN_BOUNDS=y
CONFIG_UBSAN_BOUNDS_STRICT=y
# CONFIG_UBSAN_SHIFT is not set
# CONFIG_UBSAN_DIV_ZERO is not set
# CONFIG_UBSAN_UNREACHABLE is not set
CONFIG_UBSAN_SIGNED_WRAP=y
# CONFIG_UBSAN_BOOL is not set
# CONFIG_UBSAN_ENUM is not set
# CONFIG_TEST_UBSAN is not set
Fixes: 215199e3d9f3 ("hardening: Provide Kconfig fragments for basic options")
Signed-off-by: Nathan Chancellor
---
kernel/configs/hardening.config | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/kernel/configs/hardening.config b/kernel/configs/hardening.config
index 7a5bbfc024b7..d6f6dc45628a 100644
--- a/kernel/configs/hardening.config
+++ b/kernel/configs/hardening.config
@@ -39,11 +39,11 @@ CONFIG_UBSAN=y
CONFIG_UBSAN_TRAP=y
CONFIG_UBSAN_BOUNDS=y
# CONFIG_UBSAN_SHIFT is not set
-# CONFIG_UBSAN_DIV_ZERO
-# CONFIG_UBSAN_UNREACHABLE
-# CONFIG_UBSAN_BOOL
-# CONFIG_UBSAN_ENUM
-# CONFIG_UBSAN_ALIGNMENT
+# CONFIG_UBSAN_DIV_ZERO is not set
+# CONFIG_UBSAN_UNREACHABLE is not set
+# CONFIG_UBSAN_BOOL is not set
+# CONFIG_UBSAN_ENUM is not set
+# CONFIG_UBSAN_ALIGNMENT is not set
# Sampling-based heap out-of-bounds and use-after-free detection.
CONFIG_KFENCE=y
--
2.44.0
[PATCH 0/2] configs/hardening: Some fixes for UBSAN
Hi all,
This series was spurred by a couple of recent UBSAN reports in our
continuous integration that appear to be related to
CONFIG_UBSAN_SIGNED_WRAP (which gets enabled with hardening.config due
to 'default UBSAN'), as they only appear with clang-19 and newer:
https://github.com/ClangBuiltLinux/continuous-integration2/actions/runs/8646488985/job/23709324479#step:6:500
https://github.com/ClangBuiltLinux/continuous-integration2/actions/runs/8646488985/job/23709330815#step:6:651
I'll include the information that I have gathered so far on these
specific instances below but I think that it is debatable whether
CONFIG_UBSAN_SIGNED_WRAP should be enabled by hardening.config at this
point in time, as it does not seem "production ready" to me, given that
there has not been many resources towards getting the majority of
instances cleaned up yet from what I can tell. This is particularly
problematic since hardening.config enables CONFIG_UBSAN_TRAP, so all
instances of this problem will break the kernel at runtime, which does
not seem great to me, hence patch 2. Patch 1 seems rather
uncontroversial to me :)
As for the actual crash itself, which seems like it should still be
addressed, I landed on commit 1211f3b21c2a ("workqueue: Preserve OFFQ
bits in cancel[_sync] paths") in -next for both crashes. Not immediately
obvious to me what it is complaining about though.
[0.00] Linux version 6.9.0-rc1-1-g1211f3b21c2a
(nathan@dev-arch.thelio-3990X) (ClangBuiltLinux clang version 19.0.0git
(https://github.com/llvm/llvm-project
be10070f91b86a6f126d2451852242bfcb2cd366), ClangBuiltLinux LLD 19.0.0) #1 SMP
PREEMPT Thu Apr 11 11:02:26 MST 2024
...
[0.189542] Internal error: UBSAN: unrecognized failure code:
f2005515 [#1] PREEMPT SMP
[0.193125] Modules linked in:
[0.193865] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
6.9.0-rc1-1-g1211f3b21c2a #1
[0.194185] Hardware name: linux,dummy-virt (DT)
[0.194464] pstate: 01c9 (nzcv daIF -PAN -UAO -TCO +DIT -SSBS BTYPE=--)
[0.194778] pc : cancel_delayed_work+0x54/0x94
[0.195742] lr : cancel_delayed_work+0x40/0x94
[0.195877] sp : 80008000ba30
[0.195990] x29: 80008000ba40 x28: x27:
[0.196315] x26: x25: x24:
[0.196528] x23: 9ce4d84ac000 x22: x21:
fff00294b480
[0.196746] x20: 9ce4d8c5e000 x19: 9ce4d8b28c30 x18:
80008000d058
[0.196955] x17: x16: x15:
dead0100
[0.197173] x14: 0001 x13: 0075 x12:
0a00
[0.197383] x11: fff002b10018 x10: 0008b102f0ff x9 :
7058149bb97ccd00
[0.197619] x8 : 00e1 x7 : 3d4d455453595342 x6 :
4e514553
[0.197828] x5 : fff002b1026b x4 : fff01fbdaef0 x3 :
3400
[0.198038] x2 : 80008000ba30 x1 : x0 :
[0.198326] Call trace:
[0.198544] cancel_delayed_work+0x54/0x94
[0.198810] deferred_probe_extend_timeout+0x20/0x6c
[0.198988] driver_register+0xa8/0x10c
[0.199122] __platform_driver_register+0x28/0x38
[0.199258] tegra194_cbb_init+0x24/0x34
[0.199393] do_one_initcall+0xec/0x2d0
[0.199543] do_initcall_level+0xa4/0xd0
[0.199663] do_initcalls+0x78/0xcc
[0.199770] do_basic_setup+0x24/0x34
[0.199880] kernel_init_freeable+0x110/0x180
[0.200014] kernel_init+0x28/0x1b8
[0.200123] ret_from_fork+0x10/0x20
[0.200547] Code: 5460 37f80080 39400268 371001c8 (d42aa2a0)
[0.200996] ---[ end trace ]---
---
Nathan Chancellor (2):
configs/hardening: Fix disabling UBSAN configurations
configs/hardening: Disable CONFIG_UBSAN_SIGNED_WRAP
kernel/configs/hardening.config | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
---
base-commit: fec50db7033ea478773b159e0e2efb135270e3b7
change-id: 20240410-fix-ubsan-in-hardening-config-92f66df06c4e
Best regards,
--
Nathan Chancellor
[PATCH][next] smb: smb2pdu.h: Avoid -Wflex-array-member-not-at-end warnings
-Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
ready to enable it globally.
So, in order to avoid ending up with a flexible-array member in the
middle of multiple other structs, we use the `__struct_group()` helper
to separate the flexible array from the rest of the members in the
flexible structure, and use the tagged `struct create_context_hdr`
instead of `struct create_context`.
So, with these changes, fix 51 of the following warnings[1]:
fs/smb/client/../common/smb2pdu.h:1225:31: warning: structure containing a
flexible array member is not at the end of another structure
[-Wflex-array-member-not-at-end]
Link: https://gist.github.com/GustavoARSilva/772526a39be3dd4db39e71497f0a9893
[1]
Link: https://github.com/KSPP/linux/issues/202
Signed-off-by: Gustavo A. R. Silva
---
fs/smb/client/smb2pdu.h | 12 ++--
fs/smb/common/smb2pdu.h | 33 ++---
fs/smb/server/smb2pdu.h | 18 +-
3 files changed, 33 insertions(+), 30 deletions(-)
diff --git a/fs/smb/client/smb2pdu.h b/fs/smb/client/smb2pdu.h
index c72a3b2886b7..1a02bd9e0c00 100644
--- a/fs/smb/client/smb2pdu.h
+++ b/fs/smb/client/smb2pdu.h
@@ -145,7 +145,7 @@ struct durable_context_v2 {
} __packed;
struct create_durable_v2 {
- struct create_context ccontext;
+ struct create_context_hdr ccontext;
__u8 Name[8];
struct durable_context_v2 dcontext;
} __packed;
@@ -167,7 +167,7 @@ struct durable_reconnect_context_v2_rsp {
} __packed;
struct create_durable_handle_reconnect_v2 {
- struct create_context ccontext;
+ struct create_context_hdr ccontext;
__u8 Name[8];
struct durable_reconnect_context_v2 dcontext;
__u8 Pad[4];
@@ -175,7 +175,7 @@ struct create_durable_handle_reconnect_v2 {
/* See MS-SMB2 2.2.13.2.5 */
struct crt_twarp_ctxt {
- struct create_context ccontext;
+ struct create_context_hdr ccontext;
__u8Name[8];
__le64 Timestamp;
@@ -183,12 +183,12 @@ struct crt_twarp_ctxt {
/* See MS-SMB2 2.2.13.2.9 */
struct crt_query_id_ctxt {
- struct create_context ccontext;
+ struct create_context_hdr ccontext;
__u8Name[8];
} __packed;
struct crt_sd_ctxt {
- struct create_context ccontext;
+ struct create_context_hdr ccontext;
__u8Name[8];
struct smb3_sd sd;
} __packed;
@@ -415,7 +415,7 @@ struct smb2_posix_info_parsed {
};
struct smb2_create_ea_ctx {
- struct create_context ctx;
+ struct create_context_hdr ctx;
__u8 name[8];
struct smb2_file_full_ea_info ea;
} __packed;
diff --git a/fs/smb/common/smb2pdu.h b/fs/smb/common/smb2pdu.h
index 1b594307c9d5..eab9d49c63ba 100644
--- a/fs/smb/common/smb2pdu.h
+++ b/fs/smb/common/smb2pdu.h
@@ -1171,12 +1171,15 @@ struct smb2_server_client_notification {
#define SMB2_CREATE_FLAG_REPARSEPOINT 0x01
struct create_context {
- __le32 Next;
- __le16 NameOffset;
- __le16 NameLength;
- __le16 Reserved;
- __le16 DataOffset;
- __le32 DataLength;
+ /* New members must be added within the struct_group() macro below. */
+ __struct_group(create_context_hdr, hdr, __packed,
+ __le32 Next;
+ __le16 NameOffset;
+ __le16 NameLength;
+ __le16 Reserved;
+ __le16 DataOffset;
+ __le32 DataLength;
+ );
__u8 Buffer[];
} __packed;
@@ -1222,7 +1225,7 @@ struct smb2_create_rsp {
} __packed;
struct create_posix {
- struct create_context ccontext;
+ struct create_context_hdr ccontext;
__u8Name[16];
__le32 Mode;
__u32 Reserved;
@@ -1230,7 +1233,7 @@ struct create_posix {
/* See MS-SMB2 2.2.13.2.3 and MS-SMB2 2.2.13.2.4 */
struct create_durable {
- struct create_context ccontext;
+ struct create_context_hdr ccontext;
__u8 Name[8];
union {
__u8 Reserved[16];
@@ -1243,14 +1246,14 @@ struct create_durable {
/* See MS-SMB2 2.2.13.2.5 */
struct create_mxac_req {
- struct create_context ccontext;
+ struct create_context_hdr ccontext;
__u8 Name[8];
__le64 Timestamp;
} __packed;
/* See MS-SMB2 2.2.14.2.5 */
struct create_mxac_rsp {
- struct create_context ccontext;
+ struct create_context_hdr ccontext;
__u8 Name[8];
__le32 QueryStatus;
__le32 MaximalAccess;
@@ -1286,13 +1289,13 @@ struct lease_context_v2 {
} __packed;
struct create_lease {
- struct create_context ccontext;
+ struct create_context_hdr ccontext;
__u8 Name[8];
struct lease_context lcontext;
} __packed;
struct create_lease_v2 {
- struct create_context ccontext;
+ struct create_context_hdr ccontext;
__u8 Name[8];
struct lease_context_v2 lcontext;
__u8 Pad[4];
@@ -1300,7 +1303,7 @@ struct create_lease_v2 {
/* See
Re: [PATCH] xfs: replace deprecated strncpy with strscpy_pad
On Wed, Apr 10, 2024 at 01:45:21PM -0700, Justin Stitt wrote: > On Tue, Apr 9, 2024 at 9:22 AM Kees Cook wrote: > > > > > > - /* 1 larger than sb_fname, so this ensures a trailing NUL char */ > > > - memset(label, 0, sizeof(label)); > > > spin_lock(>m_sb_lock); > > > - strncpy(label, sbp->sb_fname, XFSLABEL_MAX); > > > + strscpy_pad(label, sbp->sb_fname); > > > > Is sbp->sb_fname itself NUL-terminated? This looks like another case of > > needing the memtostr() helper? > > > > I sent a patch [1]. > > Obviously it depends on your implementation patch landing first; what > tree should it go to? This "flavor" of conversion may need to wait a release? There's no urgency on the conversion, and there are plenty more to do for this cycle. ;) -Kees > [1]: > https://lore.kernel.org/r/20240410-strncpy-xfs-split1-v2-1-7c651502b...@google.com -- Kees Cook
Re: [PATCH v3 0/3] RDMA/mana_ib: Add flex array to struct mana_cfg_rx_steer_req_v2
Hello: This series was applied to netdev/net-next.git (main) by Leon Romanovsky : On Sat, 6 Apr 2024 16:23:34 +0200 you wrote: > The "struct mana_cfg_rx_steer_req_v2" uses a dynamically sized set of > trailing elements. Specifically, it uses a "mana_handle_t" array. So, > use the preferred way in the kernel declaring a flexible array [1]. > > At the same time, prepare for the coming implementation by GCC and Clang > of the __counted_by attribute. Flexible array members annotated with > __counted_by can have their accesses bounds-checked at run-time via > CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for > strcpy/memcpy-family functions). > > [...] Here is the summary with links: - [v3,1/3] net: mana: Add flex array to struct mana_cfg_rx_steer_req_v2 https://git.kernel.org/netdev/net-next/c/bfec4e18f943 - [v3,2/3] RDMA/mana_ib: Prefer struct_size over open coded arithmetic https://git.kernel.org/netdev/net-next/c/29b8e13a8b4c - [v3,3/3] net: mana: Avoid open coded arithmetic https://git.kernel.org/netdev/net-next/c/a68292eb4316 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Re: [PATCH v3 0/3] RDMA/mana_ib: Add flex array to struct mana_cfg_rx_steer_req_v2
On Thu, 11 Apr 2024 13:58:39 +0300 Leon Romanovsky wrote: > I prepared mana-ib-flex branch > https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/log/?h=mana-ib-flex > and merge ti to our wip branch > https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/commit/?h=wip/leon-for-next=e537deecda03e0911e9406095ccd48bd42f328c7 Thanks!
Re: [PATCH v2 2/3] leds: sy7802: Add support for Silergy SY7802 flash LED controller
On Mon, 01 Apr 2024, André Apitzsch via B4 Relay wrote:
> From: André Apitzsch
>
> Add support for SY7802 flash LED controller. It can support up to 1.8A
> flash current.
This is a very small commit message for a 500+ line change!
Please, tell us more.
> Signed-off-by: André Apitzsch
> ---
> drivers/leds/flash/Kconfig | 11 +
> drivers/leds/flash/Makefile | 1 +
> drivers/leds/flash/leds-sy7802.c | 532
> +++
> 3 files changed, 544 insertions(+)
>
> diff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig
> index 809b6d98bb3e..f39f0bfe6eef 100644
> --- a/drivers/leds/flash/Kconfig
> +++ b/drivers/leds/flash/Kconfig
> @@ -121,4 +121,15 @@ config LEDS_SGM3140
> This option enables support for the SGM3140 500mA Buck/Boost Charge
> Pump LED Driver.
>
> +config LEDS_SY7802
> + tristate "LED support for the Silergy SY7802"
> + depends on I2C && OF
> + depends on GPIOLIB
> + select REGMAP_I2C
> + help
> + This option enables support for the SY7802 flash LED controller.
> + SY7802 includes torch and flash functions with programmable current.
> +
> + This driver can be built as a module, it will be called "leds-sy7802".
> +
> endif # LEDS_CLASS_FLASH
> diff --git a/drivers/leds/flash/Makefile b/drivers/leds/flash/Makefile
> index 91d60a4b7952..48860eeced79 100644
> --- a/drivers/leds/flash/Makefile
> +++ b/drivers/leds/flash/Makefile
> @@ -11,3 +11,4 @@ obj-$(CONFIG_LEDS_QCOM_FLASH) += leds-qcom-flash.o
> obj-$(CONFIG_LEDS_RT4505)+= leds-rt4505.o
> obj-$(CONFIG_LEDS_RT8515)+= leds-rt8515.o
> obj-$(CONFIG_LEDS_SGM3140) += leds-sgm3140.o
> +obj-$(CONFIG_LEDS_SY7802)+= leds-sy7802.o
> diff --git a/drivers/leds/flash/leds-sy7802.c
> b/drivers/leds/flash/leds-sy7802.c
> new file mode 100644
> index ..c03a571b0e08
> --- /dev/null
> +++ b/drivers/leds/flash/leds-sy7802.c
> @@ -0,0 +1,532 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Silergy SY7802 flash LED driver with I2C interface
> + *
> + * Copyright 2024 André Apitzsch
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define SY7802_MAX_LEDS 2
> +#define SY7802_LED_JOINT 2
> +
> +#define SY7802_REG_ENABLE0x10
> +#define SY7802_REG_TORCH_BRIGHTNESS 0xa0
> +#define SY7802_REG_FLASH_BRIGHTNESS 0xb0
> +#define SY7802_REG_FLASH_DURATION0xc0
> +#define SY7802_REG_FLAGS 0xd0
> +#define SY7802_REG_CONFIG_1 0xe0
> +#define SY7802_REG_CONFIG_2 0xf0
> +#define SY7802_REG_VIN_MONITOR 0x80
> +#define SY7802_REG_LAST_FLASH0x81
> +#define SY7802_REG_VLED_MONITOR 0x30
> +#define SY7802_REG_ADC_DELAY 0x31
> +#define SY7802_REG_DEV_ID0xff
> +
> +#define SY7802_MODE_OFF 0
> +#define SY7802_MODE_TORCH2
> +#define SY7802_MODE_FLASH3
> +#define SY7802_MODE_MASK GENMASK(1, 0)
> +
> +#define SY7802_LEDS_SHIFT3
> +#define SY7802_LEDS_MASK(_id)(BIT(_id) << SY7802_LEDS_SHIFT)
> +#define SY7802_LEDS_MASK_ALL (SY7802_LEDS_MASK(0) | SY7802_LEDS_MASK(1))
> +
> +#define SY7802_TORCH_CURRENT_SHIFT 3
> +#define SY7802_TORCH_CURRENT_MASK(_id) \
> + (GENMASK(2, 0) << (SY7802_TORCH_CURRENT_SHIFT * (_id)))
> +#define SY7802_TORCH_CURRENT_MASK_ALL \
> + (SY7802_TORCH_CURRENT_MASK(0) | SY7802_TORCH_CURRENT_MASK(1))
> +
> +#define SY7802_FLASH_CURRENT_SHIFT 4
> +#define SY7802_FLASH_CURRENT_MASK(_id) \
> + (GENMASK(3, 0) << (SY7802_FLASH_CURRENT_SHIFT * (_id)))
> +#define SY7802_FLASH_CURRENT_MASK_ALL \
> + (SY7802_FLASH_CURRENT_MASK(0) | SY7802_FLASH_CURRENT_MASK(1))
> +
> +#define SY7802_TIMEOUT_DEFAULT_US512000U
> +#define SY7802_TIMEOUT_MIN_US32000U
> +#define SY7802_TIMEOUT_MAX_US1024000U
> +#define SY7802_TIMEOUT_STEPSIZE_US 32000U
> +
> +#define SY7802_TORCH_BRIGHTNESS_MAX 8
> +
> +#define SY7802_FLASH_BRIGHTNESS_DEFAULT 14
> +#define SY7802_FLASH_BRIGHTNESS_MIN 0
> +#define SY7802_FLASH_BRIGHTNESS_MAX 15
> +#define SY7802_FLASH_BRIGHTNESS_STEP 1
Much nicer to read if everything was aligned.
> +#define SY7802_FLAG_TIMEOUT (1 << 0)
> +#define SY7802_FLAG_THERMAL_SHUTDOWN (1 << 1)
> +#define SY7802_FLAG_LED_FAULT(1 << 2)
> +#define SY7802_FLAG_TX1_INTERRUPT(1 << 3)
> +#define SY7802_FLAG_TX2_INTERRUPT(1 << 4)
> +#define SY7802_FLAG_LED_THERMAL_FAULT(1 << 5)
> +#define SY7802_FLAG_FLASH_INPUT_VOLTAGE_LOW (1 << 6)
> +#define SY7802_FLAG_INPUT_VOLTAGE_LOW(1 << 7)
Why not BIT()?
> +#define SY7802_CHIP_ID 0x51
> +
> +static const struct reg_default sy7802_regmap_defs[] = {
> + { SY7802_REG_ENABLE, SY7802_LEDS_MASK_ALL },
> + { SY7802_REG_TORCH_BRIGHTNESS, 0x92 },
> + { SY7802_REG_FLASH_BRIGHTNESS,
Re: [PATCH v3 0/3] RDMA/mana_ib: Add flex array to struct mana_cfg_rx_steer_req_v2
On Tue, Apr 09, 2024 at 02:44:19PM -0700, Jakub Kicinski wrote: > On Tue, 9 Apr 2024 18:01:40 +0100 Edward Cree wrote: > > > Shared branch would be good. Ed has some outstanding patches > > > to refactor the ethtool RSS API. > > > > For the record I am extremely unlikely to have time to get those > > done this cycle :( > > Though in any case fwiw it doesn't look like this series touches > > anything that would conflict; mana doesn't appear to support > > custom RSS contexts and besides the changes are well away from > > the ethtool API handling. > > Better safe than sorry, since the change applies cleanly on an -rc tag > having it applied to both trees should be very little extra work. I prepared mana-ib-flex branch https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/log/?h=mana-ib-flex and merge ti to our wip branch https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/commit/?h=wip/leon-for-next=e537deecda03e0911e9406095ccd48bd42f328c7 Thanks
Re: [PATCH v4] checkpatch: add check for snprintf to scnprintf
On Mon, 08 Apr 2024, Justin Stitt wrote:
> I am going to quote Lee Jones who has been doing some snprintf ->
> scnprintf refactorings:
>
> "There is a general misunderstanding amongst engineers that
> {v}snprintf() returns the length of the data *actually* encoded into the
> destination array. However, as per the C99 standard {v}snprintf()
> really returns the length of the data that *would have been* written if
> there were enough space for it. This misunderstanding has led to
> buffer-overruns in the past. It's generally considered safer to use the
> {v}scnprintf() variants in their place (or even sprintf() in simple
> cases). So let's do that."
>
> To help prevent new instances of snprintf() from popping up, let's add a
> check to checkpatch.pl.
>
> Suggested-by: Finn Thain
> Signed-off-by: Justin Stitt
> ---
> Changes in v4:
> - also check for vsnprintf variant (thanks Bill)
> - Link to v3:
> https://lore.kernel.org/r/20240315-snprintf-checkpatch-v3-1-a451e7664...@google.com
>
> Changes in v3:
> - fix indentation
> - add reference link (https://github.com/KSPP/linux/issues/105) (thanks Joe)
> - Link to v2:
> https://lore.kernel.org/r/20240221-snprintf-checkpatch-v2-1-9baeb59da...@google.com
>
> Changes in v2:
> - Had a vim moment and deleted a character before sending the patch.
> - Replaced the character :)
> - Link to v1:
> https://lore.kernel.org/r/20240221-snprintf-checkpatch-v1-1-3ac5025b5...@google.com
> ---
> From a discussion here [1].
>
> [1]:
> https://lore.kernel.org/all/0f9c95f9-2c14-eee6-7faf-635880edc...@linux-m68k.org/
> ---
> scripts/checkpatch.pl | 6 ++
> 1 file changed, 6 insertions(+)
Reviewed-by: Lee Jones
--
Lee Jones [李琼斯]
