Re: [PATCH] md/md-multipath: Convert "struct mpconf" to flexible array
Hi Christoph, On Mon, Dec 11, 2023 at 9:46 PM Christoph Hellwig wrote: > > On Fri, Dec 08, 2023 at 10:11:10AM -0800, Song Liu wrote: > > We marked it as deprecated about 2.5 years ago. But to be honest, > > I currently don't have a plan to remove it. I guess I should start thinking > > about it. > > Let's just kill it off ASAP. It never had a large user base and based > by dm-multipath not long after it has been added. It also doesn't > support any uniqueue hardware and has no on-disk format. Thanks for the suggestion. > If you want any blame deflected from you I'd be happy to send the patch > to remove it. Let me give it a try. I am kinda curious what gonna happen. :) Thanks, Song
Re: [PATCH v3] iio: sx9324: avoid copying property strings
On Tue, 2023-12-12 at 00:42 +, Justin Stitt wrote:
> We're doing some needless string copies when trying to assign the proper
> `prop` string. We can make `prop` a const char* and simply assign to
> string literals.
trivia:
I would have updated it like this moving the
various declarations into the case blocks
where they are used and removing a few unused
#defines
---
drivers/iio/proximity/sx9324.c | 69 +-
1 file changed, 41 insertions(+), 28 deletions(-)
diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
index ac2ed2da21ccc..c50c1108a69cc 100644
--- a/drivers/iio/proximity/sx9324.c
+++ b/drivers/iio/proximity/sx9324.c
@@ -877,17 +877,8 @@ static const struct sx_common_reg_default *
sx9324_get_default_reg(struct device *dev, int idx,
struct sx_common_reg_default *reg_def)
{
- static const char * const sx9324_rints[] = { "lowest", "low", "high",
- "highest" };
- static const char * const sx9324_csidle[] = { "hi-z", "hi-z", "gnd",
- "vdd" };
-#define SX9324_PIN_DEF "semtech,ph0-pin"
-#define SX9324_RESOLUTION_DEF "semtech,ph01-resolution"
-#define SX9324_PROXRAW_DEF "semtech,ph01-proxraw-strength"
- unsigned int pin_defs[SX9324_NUM_PINS];
- char prop[] = SX9324_PROXRAW_DEF;
- u32 start = 0, raw = 0, pos = 0;
- int ret, count, ph, pin;
+ u32 raw = 0;
+ int ret;
memcpy(reg_def, &sx9324_default_regs[idx], sizeof(*reg_def));
@@ -896,7 +887,13 @@ sx9324_get_default_reg(struct device *dev, int idx,
case SX9324_REG_AFE_PH0:
case SX9324_REG_AFE_PH1:
case SX9324_REG_AFE_PH2:
- case SX9324_REG_AFE_PH3:
+ case SX9324_REG_AFE_PH3: {
+ unsigned int pin_defs[SX9324_NUM_PINS];
+ int count;
+ int pin;
+ int ph;
+ char prop[32];
+
ph = reg_def->reg - SX9324_REG_AFE_PH0;
snprintf(prop, ARRAY_SIZE(prop), "semtech,ph%d-pin", ph);
@@ -913,7 +910,15 @@ sx9324_get_default_reg(struct device *dev, int idx,
SX9324_REG_AFE_PH0_PIN_MASK(pin);
reg_def->def = raw;
break;
- case SX9324_REG_AFE_CTRL0:
+ }
+ case SX9324_REG_AFE_CTRL0: {
+ static const char * const sx9324_csidle[] = {
+ "hi-z", "hi-z", "gnd", "vdd"
+ };
+ static const char * const sx9324_rints[] = {
+ "lowest", "low", "high", "highest"
+ };
+
ret = device_property_match_property_string(dev,
"semtech,cs-idle-sleep",
sx9324_csidle,
ARRAY_SIZE(sx9324_csidle));
@@ -930,16 +935,17 @@ sx9324_get_default_reg(struct device *dev, int idx,
reg_def->def |= ret << SX9324_REG_AFE_CTRL0_RINT_SHIFT;
}
break;
+ }
case SX9324_REG_AFE_CTRL4:
- case SX9324_REG_AFE_CTRL7:
+ case SX9324_REG_AFE_CTRL7: {
+ const char *type;
+
if (reg_def->reg == SX9324_REG_AFE_CTRL4)
- strncpy(prop, "semtech,ph01-resolution",
- ARRAY_SIZE(prop));
+ type = "semtech,ph01-resolution";
else
- strncpy(prop, "semtech,ph23-resolution",
- ARRAY_SIZE(prop));
+ type = "semtech,ph23-resolution";
- ret = device_property_read_u32(dev, prop, &raw);
+ ret = device_property_read_u32(dev, type, &raw);
if (ret)
break;
@@ -949,6 +955,7 @@ sx9324_get_default_reg(struct device *dev, int idx,
reg_def->def |= FIELD_PREP(SX9324_REG_AFE_CTRL4_RESOLUTION_MASK,
raw);
break;
+ }
case SX9324_REG_AFE_CTRL8:
ret = device_property_read_u32(dev,
"semtech,input-precharge-resistor-ohms",
@@ -982,17 +989,21 @@ sx9324_get_default_reg(struct device *dev, int idx,
6 + raw * (raw + 3) / 2);
break;
- case SX9324_REG_ADV_CTRL5:
+ case SX9324_REG_ADV_CTRL5: {
+ u32 start = 0;
+
ret = device_property_read_u32(dev, "semtech,startup-sensor",
&start);
if (ret)
break;
-
reg_def->def &= ~SX9324_REG_ADV_CTRL5_STARTUPSENS_MASK;
reg_def->def |=
FIELD_PREP(SX9324_REG_ADV_CTRL5_STARTUPSENS_MASK,
start);
break;
- case SX9324_REG_PROX_CTRL4:
+ }
+ case SX9324_REG_
Re: [PATCH] md/md-multipath: Convert "struct mpconf" to flexible array
On Fri, Dec 08, 2023 at 10:11:10AM -0800, Song Liu wrote: > We marked it as deprecated about 2.5 years ago. But to be honest, > I currently don't have a plan to remove it. I guess I should start thinking > about it. Let's just kill it off ASAP. It never had a large user base and based by dm-multipath not long after it has been added. It also doesn't support any uniqueue hardware and has no on-disk format. If you want any blame deflected from you I'd be happy to send the patch to remove it.
[PATCH v2] PCI: Prevent device from doing RPM when it's unplugged
When inserting an SD7.0 card to Realtek card reader, the card reader unplugs itself and morph into a NVMe device. The slot Link down on hot unplugged can cause the following error: [ 63.898861] pcieport :00:1c.0: pciehp: Slot(8): Link Down [ 63.912118] BUG: unable to handle page fault for address: b24d403e5010 [ 63.912122] #PF: supervisor read access in kernel mode [ 63.912125] #PF: error_code(0x) - not-present page [ 63.912126] PGD 10067 P4D 10067 PUD 1001fe067 PMD 100d97067 PTE 0 [ 63.912131] Oops: [#1] PREEMPT SMP PTI [ 63.912134] CPU: 3 PID: 534 Comm: kworker/3:10 Not tainted 6.4.0 #6 [ 63.912137] Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./H370M Pro4, BIOS P3.40 10/25/2018 [ 63.912138] Workqueue: pm pm_runtime_work [ 63.912144] RIP: 0010:ioread32+0x2e/0x70 [ 63.912148] Code: ff 03 00 77 25 48 81 ff 00 00 01 00 77 14 8b 15 08 d9 54 01 b8 ff ff ff ff 85 d2 75 14 c3 cc cc cc cc 89 fa ed c3 cc cc cc cc <8b> 07 c3 cc cc cc cc 55 83 ea 01 48 89 fe 48 c7 c7 98 6f 15 99 48 [ 63.912150] RSP: 0018:b24d40a5bd78 EFLAGS: 00010296 [ 63.912152] RAX: b24d403e5000 RBX: 0152 RCX: 007f [ 63.912153] RDX: ff00 RSI: b24d403e5010 RDI: b24d403e5010 [ 63.912155] RBP: b24d40a5bd98 R08: b24d403e5010 R09: [ 63.912156] R10: 9074cd95e7f4 R11: 0003 R12: 007f [ 63.912158] R13: 9074e1a68c00 R14: 9074e1a68d00 R15: 9003 [ 63.912159] FS: () GS:90752a18() knlGS: [ 63.912161] CS: 0010 DS: ES: CR0: 80050033 [ 63.912162] CR2: b24d403e5010 CR3: 000152832006 CR4: 003706e0 [ 63.912164] Call Trace: [ 63.912165] [ 63.912167] ? show_regs+0x68/0x70 [ 63.912171] ? __die_body+0x20/0x70 [ 63.912173] ? __die+0x2b/0x40 [ 63.912175] ? page_fault_oops+0x160/0x480 [ 63.912177] ? search_bpf_extables+0x63/0x90 [ 63.912180] ? ioread32+0x2e/0x70 [ 63.912183] ? search_exception_tables+0x5f/0x70 [ 63.912186] ? kernelmode_fixup_or_oops+0xa2/0x120 [ 63.912189] ? __bad_area_nosemaphore+0x179/0x230 [ 63.912191] ? bad_area_nosemaphore+0x16/0x20 [ 63.912193] ? do_kern_addr_fault+0x8b/0xa0 [ 63.912195] ? exc_page_fault+0xe5/0x180 [ 63.912198] ? asm_exc_page_fault+0x27/0x30 [ 63.912203] ? ioread32+0x2e/0x70 [ 63.912206] ? rtsx_pci_write_register+0x5b/0x90 [rtsx_pci] [ 63.912217] rtsx_set_l1off_sub+0x1c/0x30 [rtsx_pci] [ 63.912226] rts5261_set_l1off_cfg_sub_d0+0x36/0x40 [rtsx_pci] [ 63.912234] rtsx_pci_runtime_idle+0xc7/0x160 [rtsx_pci] [ 63.912243] ? __pfx_pci_pm_runtime_idle+0x10/0x10 [ 63.912246] pci_pm_runtime_idle+0x34/0x70 [ 63.912248] rpm_idle+0xc4/0x2b0 [ 63.912251] pm_runtime_work+0x93/0xc0 [ 63.912254] process_one_work+0x21a/0x430 [ 63.912258] worker_thread+0x4a/0x3c0 [ 63.912261] ? __pfx_worker_thread+0x10/0x10 [ 63.912263] kthread+0x106/0x140 [ 63.912266] ? __pfx_kthread+0x10/0x10 [ 63.912268] ret_from_fork+0x29/0x50 [ 63.912273] [ 63.912274] Modules linked in: nvme nvme_core snd_hda_codec_hdmi snd_sof_pci_intel_cnl snd_sof_intel_hda_common snd_hda_codec_realtek snd_hda_codec_generic snd_soc_hdac_hda soundwire_intel ledtrig_audio nls_iso8859_1 soundwire_generic_allocation soundwire_cadence snd_sof_intel_hda_mlink snd_sof_intel_hda snd_sof_pci snd_sof_xtensa_dsp snd_sof snd_sof_utils snd_hda_ext_core snd_soc_acpi_intel_match snd_soc_acpi soundwire_bus snd_soc_core snd_compress ac97_bus snd_pcm_dmaengine snd_hda_intel i915 snd_intel_dspcfg snd_intel_sdw_acpi intel_rapl_msr snd_hda_codec intel_rapl_common snd_hda_core x86_pkg_temp_thermal intel_powerclamp snd_hwdep coretemp snd_pcm kvm_intel drm_buddy ttm mei_hdcp kvm drm_display_helper snd_seq_midi snd_seq_midi_event cec crct10dif_pclmul ghash_clmulni_intel sha512_ssse3 aesni_intel crypto_simd rc_core cryptd rapl snd_rawmidi drm_kms_helper binfmt_misc intel_cstate i2c_algo_bit joydev snd_seq snd_seq_device syscopyarea wmi_bmof snd_timer sysfillrect input_leds snd ee1004 sysimgblt mei_me soundcore [ 63.912324] mei intel_pch_thermal mac_hid acpi_tad acpi_pad sch_fq_codel msr parport_pc ppdev lp ramoops drm parport reed_solomon efi_pstore ip_tables x_tables autofs4 hid_generic usbhid hid rtsx_pci_sdmmc crc32_pclmul ahci e1000e i2c_i801 i2c_smbus rtsx_pci xhci_pci libahci xhci_pci_renesas video wmi [ 63.912346] CR2: b24d403e5010 [ 63.912348] ---[ end trace ]--- This happens because scheduled pm_runtime_idle() is not cancelled. So before releasing the device, stop all runtime power managements by using pm_runtime_barrier() to fix the issue. Link: https://lore.kernel.org/all/2ce258f371234b1f8a1a470d5488d...@realtek.com/ Tested-by: Ricky Wu --- v2: Cover more cases than just pciehp. drivers/pci/remove.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/pci/remove.c b/d
[PATCH v2] scsi: ibmvscsi_tgt: replace deprecated strncpy with strscpy
strncpy() is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.
We don't need the NUL-padding behavior that strncpy() provides as vscsi
is NUL-allocated in ibmvscsis_probe() which proceeds to call
ibmvscsis_adapter_info():
| vscsi = kzalloc(sizeof(*vscsi), GFP_KERNEL);
ibmvscsis_probe() -> ibmvscsis_handle_crq() -> ibmvscsis_parse_command()
-> ibmvscsis_mad() -> ibmvscsis_process_mad() -> ibmvscsis_adapter_info()
Following the same idea, `partition_name` is defiend as:
| static char partition_name[PARTITION_NAMELEN] = "UNKNOWN";
... which is NUL-padded already, meaning strscpy() is the best option.
Considering the above, a suitable replacement is `strscpy` [2] due to
the fact that it guarantees NUL-termination on the destination buffer
without unnecessarily NUL-padding.
However, for cap->name and info let's use strscpy_pad as they are
allocated via dma_alloc_coherent():
| cap = dma_alloc_coherent(&vscsi->dma_dev->dev, olen, &token,
|GFP_ATOMIC);
&
| info = dma_alloc_coherent(&vscsi->dma_dev->dev, sizeof(*info), &token,
| GFP_ATOMIC);
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt
---
Changes in v2:
- use strscpy_pad for info->partition_name (thanks Kees)
- rebase onto mainline bee0e7762ad2c602
- Link to v1:
https://lore.kernel.org/r/20231030-strncpy-drivers-scsi-ibmvscsi_tgt-ibmvscsi_tgt-c-v1-1-859b5ce25...@google.com
---
Note: build-tested only.
Found with: $ rg "strncpy\("
---
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index 4dc411a58107..6b16020b1f59 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -1551,18 +1551,18 @@ static long ibmvscsis_adapter_info(struct scsi_info
*vscsi,
if (vscsi->client_data.partition_number == 0)
vscsi->client_data.partition_number =
be32_to_cpu(info->partition_number);
- strncpy(vscsi->client_data.srp_version, info->srp_version,
+ strscpy(vscsi->client_data.srp_version, info->srp_version,
sizeof(vscsi->client_data.srp_version));
- strncpy(vscsi->client_data.partition_name, info->partition_name,
+ strscpy(vscsi->client_data.partition_name, info->partition_name,
sizeof(vscsi->client_data.partition_name));
vscsi->client_data.mad_version = be32_to_cpu(info->mad_version);
vscsi->client_data.os_type = be32_to_cpu(info->os_type);
/* Copy our info */
- strncpy(info->srp_version, SRP_VERSION,
- sizeof(info->srp_version));
- strncpy(info->partition_name, vscsi->dds.partition_name,
- sizeof(info->partition_name));
+ strscpy_pad(info->srp_version, SRP_VERSION,
+ sizeof(info->srp_version));
+ strscpy_pad(info->partition_name, vscsi->dds.partition_name,
+ sizeof(info->partition_name));
info->partition_number = cpu_to_be32(vscsi->dds.partition_num);
info->mad_version = cpu_to_be32(MAD_VERSION_1);
info->os_type = cpu_to_be32(LINUX);
@@ -1645,8 +1645,8 @@ static int ibmvscsis_cap_mad(struct scsi_info *vscsi,
struct iu_entry *iue)
be64_to_cpu(mad->buffer),
vscsi->dds.window[LOCAL].liobn, token);
if (rc == H_SUCCESS) {
- strncpy(cap->name, dev_name(&vscsi->dma_dev->dev),
- SRP_MAX_LOC_LEN);
+ strscpy_pad(cap->name, dev_name(&vscsi->dma_dev->dev),
+ sizeof(cap->name));
len = olen - min_len;
status = VIOSRP_MAD_SUCCESS;
@@ -3650,7 +3650,7 @@ static int ibmvscsis_get_system_info(void)
name = of_get_property(rootdn, "ibm,partition-name", NULL);
if (name)
- strncpy(partition_name, name, sizeof(partition_name));
+ strscpy(partition_name, name, sizeof(partition_name));
num = of_get_property(rootdn, "ibm,partition-no", NULL);
if (num)
---
base-commit: bee0e7762ad2c6025b9f5245c040fcc36ef2bde8
change-id:
20231030-strncpy-drivers-scsi-ibmvscsi_tgt-ibmvscsi_tgt-c-8a9bd0e54666
Best regards,
--
Justin Stitt
Re: [PATCH v2] iio: sx9324: avoid copying property strings
Hi,
On Mon, Oct 30, 2023 at 2:44 PM Stephen Boyd wrote:
>
>
> We need to free it in other places too, like if the count doesn't match.
> It may be easier to extract this section and just have 4 string
> literals.
>
> switch (reg_def->reg) {
> case SX9324_REG_AFE_PH0:
> reg_def = sx9324_parse_phase_prop(dev, reg_def,
> "semtech,ph0-pin");
> break;
> case SX9324_REG_AFE_PH1:
> reg_def = sx9324_parse_phase_prop(dev, reg_def,
> "semtech,ph1-pin");
> break;
> case SX9324_REG_AFE_PH2:
> reg_def = sx9324_parse_phase_prop(dev, reg_def,
> "semtech,ph2-pin");
> break;
> case SX9324_REG_AFE_PH3:
> reg_def = sx9324_parse_phase_prop(dev, reg_def,
> "semtech,ph3-pin");
> break;
>
I've submitted v3 of this patch [1] trying out Stephen's idea. I'd
appreciate feedback.
[1]:
https://lore.kernel.org/all/20231212-strncpy-drivers-iio-proximity-sx9324-c-v3-1-b8ae12fc8...@google.com/
Thanks
Justin
[PATCH v3] iio: sx9324: avoid copying property strings
We're doing some needless string copies when trying to assign the proper
`prop` string. We can make `prop` a const char* and simply assign to
string literals.
For the case where a format string is used, let's extract the parsing
logic out into sx9324_parse_phase_prop(). We no longer need to create
copies or allocate new memory.
sx9324_parse_phase_prop() will simply return the default def value if it
fails.
This also cleans up some deprecated strncpy() uses [1].
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[1]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt
---
Changes in v3:
- extract logic into sx9324_parse_phase_prop() and use string literals
(thanks Stephen)
- rebase onto mainline bee0e7762ad2c602
- Link to v2:
https://lore.kernel.org/r/20231026-strncpy-drivers-iio-proximity-sx9324-c-v2-1-cee6e5db7...@google.com
Changes in v2:
- make prop a const char* and do simple assignments (thanks Jonathan)
- rebase onto 3a568e3a961ba330
- Link to v1:
https://lore.kernel.org/r/20230921-strncpy-drivers-iio-proximity-sx9324-c-v1-1-4e8d28fd1...@google.com
---
---
drivers/iio/proximity/sx9324.c | 69 ++
1 file changed, 43 insertions(+), 26 deletions(-)
diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
index 438f9c9aba6e..e3bc30b57b19 100644
--- a/drivers/iio/proximity/sx9324.c
+++ b/drivers/iio/proximity/sx9324.c
@@ -873,6 +873,32 @@ static int sx9324_init_compensation(struct iio_dev
*indio_dev)
2, 200);
}
+static u32 sx9324_parse_phase_prop(struct device *dev,
+ struct sx_common_reg_default *reg_def,
+ int idx, const char *prop)
+{
+ unsigned int pin_defs[SX9324_NUM_PINS];
+ int count, ret, pin;
+ u8 default_def;
+ u32 raw = 0;
+
+ default_def = sx9324_default_regs[idx].def;
+
+ count = device_property_count_u32(dev, prop);
+ if (count != ARRAY_SIZE(pin_defs))
+ return default_def;
+ ret = device_property_read_u32_array(dev, prop, pin_defs,
+ARRAY_SIZE(pin_defs));
+ if (ret)
+ return default_def;
+
+ for (pin = 0; pin < SX9324_NUM_PINS; pin++)
+ raw |= (pin_defs[pin] << (2 * pin)) &
+ SX9324_REG_AFE_PH0_PIN_MASK(pin);
+
+ return raw;
+}
+
static const struct sx_common_reg_default *
sx9324_get_default_reg(struct device *dev, int idx,
struct sx_common_reg_default *reg_def)
@@ -884,35 +910,30 @@ sx9324_get_default_reg(struct device *dev, int idx,
#define SX9324_PIN_DEF "semtech,ph0-pin"
#define SX9324_RESOLUTION_DEF "semtech,ph01-resolution"
#define SX9324_PROXRAW_DEF "semtech,ph01-proxraw-strength"
- unsigned int pin_defs[SX9324_NUM_PINS];
- char prop[] = SX9324_PROXRAW_DEF;
+ const char *prop = SX9324_PROXRAW_DEF;
u32 start = 0, raw = 0, pos = 0;
- int ret, count, ph, pin;
const char *res;
+ int ret;
memcpy(reg_def, &sx9324_default_regs[idx], sizeof(*reg_def));
sx_common_get_raw_register_config(dev, reg_def);
switch (reg_def->reg) {
case SX9324_REG_AFE_PH0:
+ reg_def->def = sx9324_parse_phase_prop(dev, reg_def, idx,
+ "semtech,ph0-pin");
+ break;
case SX9324_REG_AFE_PH1:
+ reg_def->def = sx9324_parse_phase_prop(dev, reg_def, idx,
+ "semtech,ph1-pin");
+ break;
case SX9324_REG_AFE_PH2:
+ reg_def->def = sx9324_parse_phase_prop(dev, reg_def, idx,
+ "semtech,ph2-pin");
+ break;
case SX9324_REG_AFE_PH3:
- ph = reg_def->reg - SX9324_REG_AFE_PH0;
- snprintf(prop, ARRAY_SIZE(prop), "semtech,ph%d-pin", ph);
-
- count = device_property_count_u32(dev, prop);
- if (count != ARRAY_SIZE(pin_defs))
- break;
- ret = device_property_read_u32_array(dev, prop, pin_defs,
-ARRAY_SIZE(pin_defs));
- if (ret)
- break;
-
- for (pin = 0; pin < SX9324_NUM_PINS; pin++)
- raw |= (pin_defs[pin] << (2 * pin)) &
- SX9324_REG_AFE_PH0_PIN_MASK(pin);
- reg_def->def = raw;
+ reg_def->def = sx9324_parse_phase_prop(dev, reg_def, idx,
+ "semtech,ph3-pin");
break;
case SX9324_REG_AFE_CTRL0:
ret = device_property_read_string(dev,
@@ -937,11 +958,9
Re: [PATCH v3] net: mdio-gpio: replace deprecated strncpy with strscpy
On Mon, Dec 11, 2023 at 07:10:00PM +, Justin Stitt wrote: > strncpy() is deprecated for use on NUL-terminated destination strings > [1] and as such we should prefer more robust and less ambiguous string > interfaces. > > We expect new_bus->id to be NUL-terminated but not NUL-padded based on > its prior assignment through snprintf: > | snprintf(new_bus->id, MII_BUS_ID_SIZE, "gpio-%x", bus_id); > > Due to this, a suitable replacement is `strscpy` [2] due to the fact > that it guarantees NUL-termination on the destination buffer without > unnecessarily NUL-padding. > > We can also use sizeof() instead of a length macro as this more closely > ties the maximum buffer size to the destination buffer. Do this for two > instances. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt Reviewed-by: Russell King (Oracle) Thanks! -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH] lib/string: shrink lib/string.i via IWYU
On Mon, Dec 11, 2023 at 10:48 PM Nick Desaulniers wrote: > On Tue, Dec 5, 2023 at 1:44 PM Al Viro wrote: > > On Tue, Dec 05, 2023 at 01:39:47PM -0800, Nick Desaulniers wrote: > > > > > The tooling Tanzir is working on does wrap IWYU, and does support such > > > mapping (of 'low level' to 'high level' headers; more so, if it > > > recommends X you can override to suggest Y instead). > > > > > > arch/nios/ also doesn't provide a bug.h, which this patch is > > > suggesting we include directly. I guess the same goes for > > > asm/rwonce.h. > > > > See include/asm-generic/Kbuild: > > mandatory-y += bug.h > > ... > > mandatory-y += rwonce.h > > > > IOW, sh will have asm/bug.h and as/rwonce.h copied from asm-generic. > > > > Still, includes of asm/*.h had been a massive headache historically > > and breeding more of those shouldn't be overdone. > > > > More painful problem is arch- and config-dependent stuff, though... > > Ah, it looks like include/uapi/asm-generic/Kbuild also makes use of > this pattern using mandatory-y. > > So I think we can handle this as a two step translation. We can tell > the tooling to 'nevermind recommending X, always replace > recommendations for X with Y ', so: > 1. any recommendation to use asm-generic/foo.h should be replaced with > asm/foo.h (always, based on those 2 Kbuild files, could autogenerate) > 2. some recommendations to use asm/foo.h should be replaced with > linux/foo.h (not necessarily codified anywhere; depends on if there is > a linux/foo.h, will manually curate this list for now) > > Orthogonal but some places in tree can be cleaned up to include > linux/foo.h rather than asm/foo.h. > > Does this sound like an improvement to my mental model of the > conventions used for kernel header inclusion within the linux kernel? > > Tanzir nearly has the above done (for 1, 2 we will probably need to > iterate on more). We've also beefed up our local testing to test more > architectures. I expect Tanzir to send a v2 of this patch (as a > series, with the fix for ARCH=sh) later this week, if the above seems > correct. Whatever you choose, please make sure it gets documented, so we won't go another round in the future for something similar. -- With Best Regards, Andy Shevchenko
Re: [PATCH] lib/string: shrink lib/string.i via IWYU
On Tue, Dec 5, 2023 at 1:44 PM Al Viro wrote: > > On Tue, Dec 05, 2023 at 01:39:47PM -0800, Nick Desaulniers wrote: > > > The tooling Tanzir is working on does wrap IWYU, and does support such > > mapping (of 'low level' to 'high level' headers; more so, if it > > recommends X you can override to suggest Y instead). > > > > arch/nios/ also doesn't provide a bug.h, which this patch is > > suggesting we include directly. I guess the same goes for > > asm/rwonce.h. > > See include/asm-generic/Kbuild: > mandatory-y += bug.h > ... > mandatory-y += rwonce.h > > IOW, sh will have asm/bug.h and as/rwonce.h copied from asm-generic. > > Still, includes of asm/*.h had been a massive headache historically > and breeding more of those shouldn't be overdone. > > More painful problem is arch- and config-dependent stuff, though... Ah, it looks like include/uapi/asm-generic/Kbuild also makes use of this pattern using mandatory-y. So I think we can handle this as a two step translation. We can tell the tooling to 'nevermind recommending X, always replace recommendations for X with Y ', so: 1. any recommendation to use asm-generic/foo.h should be replaced with asm/foo.h (always, based on those 2 Kbuild files, could autogenerate) 2. some recommendations to use asm/foo.h should be replaced with linux/foo.h (not necessarily codified anywhere; depends on if there is a linux/foo.h, will manually curate this list for now) Orthogonal but some places in tree can be cleaned up to include linux/foo.h rather than asm/foo.h. Does this sound like an improvement to my mental model of the conventions used for kernel header inclusion within the linux kernel? Tanzir nearly has the above done (for 1, 2 we will probably need to iterate on more). We've also beefed up our local testing to test more architectures. I expect Tanzir to send a v2 of this patch (as a series, with the fix for ARCH=sh) later this week, if the above seems correct. -- Thanks, ~Nick Desaulniers
Re: [PATCH] scsi: fcoe: replace deprecated strncpy with strscpy
On 12/11/23 12:08, Justin Stitt wrote:
Hi,
On Tue, Oct 24, 2023 at 1:01 PM Bart Van Assche wrote:
On 10/24/23 12:52, Justin Stitt wrote:
diff --git a/drivers/scsi/fcoe/fcoe_sysfs.c b/drivers/scsi/fcoe/fcoe_sysfs.c
index e17957f8085c..7a3ca6cd3030 100644
--- a/drivers/scsi/fcoe/fcoe_sysfs.c
+++ b/drivers/scsi/fcoe/fcoe_sysfs.c
@@ -279,12 +279,10 @@ static ssize_t store_ctlr_mode(struct device *dev,
if (count > FCOE_MAX_MODENAME_LEN)
return -EINVAL;
- strncpy(mode, buf, count);
+ strscpy(mode, buf, count);
if (mode[count - 1] == '\n')
mode[count - 1] = '\0';
- else
- mode[count] = '\0';
switch (ctlr->enabled) {
case FCOE_CTLR_ENABLED:
Please consider to remove the code for copying the sysfs string and to
use sysfs_match_string() instead.
Sorry, I'm not too familiar with sysfs strings here.
Let me know what you think of this patch [1].
I don't use FCoE so I will leave it to an FCoE user to review that patch.
Thanks,
Bart.
Re: [PATCH] scsi: fcoe: replace deprecated strncpy with strscpy
Hi,
On Tue, Oct 24, 2023 at 1:01 PM Bart Van Assche wrote:
>
> On 10/24/23 12:52, Justin Stitt wrote:
> > diff --git a/drivers/scsi/fcoe/fcoe_sysfs.c b/drivers/scsi/fcoe/fcoe_sysfs.c
> > index e17957f8085c..7a3ca6cd3030 100644
> > --- a/drivers/scsi/fcoe/fcoe_sysfs.c
> > +++ b/drivers/scsi/fcoe/fcoe_sysfs.c
> > @@ -279,12 +279,10 @@ static ssize_t store_ctlr_mode(struct device *dev,
> > if (count > FCOE_MAX_MODENAME_LEN)
> > return -EINVAL;
> >
> > - strncpy(mode, buf, count);
> > + strscpy(mode, buf, count);
> >
> > if (mode[count - 1] == '\n')
> > mode[count - 1] = '\0';
> > - else
> > - mode[count] = '\0';
> >
> > switch (ctlr->enabled) {
> > case FCOE_CTLR_ENABLED:
>
> Please consider to remove the code for copying the sysfs string and to
> use sysfs_match_string() instead.
>
Sorry, I'm not too familiar with sysfs strings here.
Let me know what you think of this patch [1].
> Thanks,
>
> Bart.
>
[1]:
https://lore.kernel.org/r/20231211-strncpy-drivers-scsi-fcoe-fcoe_sysfs-c-v1-1-73b942238...@google.com
Thanks
Justin
[PATCH] scsi: fcoe: use sysfs_match_string over fcoe_parse_mode
Instead of copying @buf into a new buffer and carefully managing its
newline/null-terminating status, we can just use sysfs_match_string()
as it uses sysfs_streq() internally which handles newline/null-term:
| /**
| * sysfs_streq - return true if strings are equal, modulo trailing newline
| * @s1: one string
| * @s2: another string
| *
| * This routine returns true iff two strings are equal, treating both
| * NUL and newline-then-NUL as equivalent string terminations. It's
| * geared for use with sysfs input strings, which generally terminate
| * with newlines but are compared against values without newlines.
| */
| bool sysfs_streq(const char *s1, const char *s2)
| ...
Then entirely drop the now unused fcoe_parse_mode, being careful to
change if condition from checking for FIP_CONN_TYPE_UNKNOWN to < 0 as
sysfs_match_string can return -EINVAL.
To get the compiler not to complain, make fip_conn_type_names
const char * const. Perhaps, this should also be done for
fcf_state_names.
This also removes an instance of strncpy() which helps [1].
Link: https://github.com/KSPP/linux/issues/90 [1]
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt
---
Builds upon patch and feedback from [2]:
However, this is different enough to warrant its own patch and not be a
continuation.
[2]: https://lore.kernel.org/all/9f38f4aa-c6b5-4786-a641-d02d8bd92...@acm.org/
---
drivers/scsi/fcoe/fcoe_sysfs.c | 26 --
1 file changed, 4 insertions(+), 22 deletions(-)
diff --git a/drivers/scsi/fcoe/fcoe_sysfs.c b/drivers/scsi/fcoe/fcoe_sysfs.c
index e17957f8085c..f9c5d00f658a 100644
--- a/drivers/scsi/fcoe/fcoe_sysfs.c
+++ b/drivers/scsi/fcoe/fcoe_sysfs.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include
#include
@@ -214,25 +215,13 @@ static const char *get_fcoe_##title##_name(enum
table_type table_key) \
return table[table_key];\
}
-static char *fip_conn_type_names[] = {
+static const char * const fip_conn_type_names[] = {
[ FIP_CONN_TYPE_UNKNOWN ] = "Unknown",
[ FIP_CONN_TYPE_FABRIC ] = "Fabric",
[ FIP_CONN_TYPE_VN2VN ] = "VN2VN",
};
fcoe_enum_name_search(ctlr_mode, fip_conn_type, fip_conn_type_names)
-static enum fip_conn_type fcoe_parse_mode(const char *buf)
-{
- int i;
-
- for (i = 0; i < ARRAY_SIZE(fip_conn_type_names); i++) {
- if (strcasecmp(buf, fip_conn_type_names[i]) == 0)
- return i;
- }
-
- return FIP_CONN_TYPE_UNKNOWN;
-}
-
static char *fcf_state_names[] = {
[ FCOE_FCF_STATE_UNKNOWN ] = "Unknown",
[ FCOE_FCF_STATE_DISCONNECTED ] = "Disconnected",
@@ -274,17 +263,10 @@ static ssize_t store_ctlr_mode(struct device *dev,
const char *buf, size_t count)
{
struct fcoe_ctlr_device *ctlr = dev_to_ctlr(dev);
- char mode[FCOE_MAX_MODENAME_LEN + 1];
if (count > FCOE_MAX_MODENAME_LEN)
return -EINVAL;
- strncpy(mode, buf, count);
-
- if (mode[count - 1] == '\n')
- mode[count - 1] = '\0';
- else
- mode[count] = '\0';
switch (ctlr->enabled) {
case FCOE_CTLR_ENABLED:
@@ -297,8 +279,8 @@ static ssize_t store_ctlr_mode(struct device *dev,
return -ENOTSUPP;
}
- ctlr->mode = fcoe_parse_mode(mode);
- if (ctlr->mode == FIP_CONN_TYPE_UNKNOWN) {
+ ctlr->mode = sysfs_match_string(fip_conn_type_names, buf);
+ if (ctlr->mode < 0) {
LIBFCOE_SYSFS_DBG(ctlr, "Unknown mode %s provided.\n",
buf);
return -EINVAL;
---
base-commit: bee0e7762ad2c6025b9f5245c040fcc36ef2bde8
change-id: 20231024-strncpy-drivers-scsi-fcoe-fcoe_sysfs-c-0e1dffe82855
Best regards,
--
Justin Stitt
Re: [PATCH v2] net: mdio-gpio: replace deprecated strncpy with strscpy
On Thu, Dec 7, 2023 at 2:57 PM Russell King (Oracle) wrote: > > On Thu, Dec 07, 2023 at 09:54:31PM +, Justin Stitt wrote: > > We expect new_bus->id to be NUL-terminated but not NUL-padded based on > > its prior assignment through snprintf: > > | snprintf(new_bus->id, MII_BUS_ID_SIZE, "gpio-%x", bus_id); > > > > We can also use sizeof() instead of a length macro as this more closely > > ties the maximum buffer size to the destination buffer. > > Honestly, this looks machine generated and unreviewed by the submitter, > because... > Not machine generated. Was just trying to keep my change as small as possible towards the goal of replacing strncpy. However, you're right. It's literally the line right above it and now it looks inconsistent . > > if (bus_id != -1) > > snprintf(new_bus->id, MII_BUS_ID_SIZE, "gpio-%x", bus_id); > > else > > - strncpy(new_bus->id, "gpio", MII_BUS_ID_SIZE); > > + strscpy(new_bus->id, "gpio", sizeof(new_bus->id)); > > If there is an argument for not using MII_BUS_ID_SIZE in one place, > then the very same argument applies to snprintf(). If one place > changes the other also needs to be changed. > Gotcha, I've sent a [v3]. > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! [v3]: https://lore.kernel.org/all/20231211-strncpy-drivers-net-mdio-mdio-gpio-c-v3-1-76dea53a1...@google.com/ Thanks Justin
[PATCH v3] net: mdio-gpio: replace deprecated strncpy with strscpy
strncpy() is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.
We expect new_bus->id to be NUL-terminated but not NUL-padded based on
its prior assignment through snprintf:
| snprintf(new_bus->id, MII_BUS_ID_SIZE, "gpio-%x", bus_id);
Due to this, a suitable replacement is `strscpy` [2] due to the fact
that it guarantees NUL-termination on the destination buffer without
unnecessarily NUL-padding.
We can also use sizeof() instead of a length macro as this more closely
ties the maximum buffer size to the destination buffer. Do this for two
instances.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt
---
Changes in v3:
- swap another instance of MII_BUS_ID_SIZE to sizeof() (thanks Russell)
- rebase onto mainline bee0e7762ad2c602
- Link to v2:
https://lore.kernel.org/r/20231207-strncpy-drivers-net-mdio-mdio-gpio-c-v2-1-c28d52dd3...@google.com
Changes in v2:
- change subject line as it was causing problems in patchwork with
"superseded" label being improperly applied.
- update commit msg with rationale around sizeof() (thanks Kees)
- Link to v1 (lore):
https://lore.kernel.org/r/20231012-strncpy-drivers-net-mdio-mdio-gpio-c-v1-1-ab9b06cfc...@google.com
- Link to v1 (patchwork):
https://patchwork.kernel.org/project/netdevbpf/patch/20231012-strncpy-drivers-net-mdio-mdio-gpio-c-v1-1-ab9b06cfc...@google.com/
- Link to other patch with same subject message:
https://patchwork.kernel.org/project/netdevbpf/patch/20231012-strncpy-drivers-net-phy-mdio_bus-c-v1-1-15242e6f9...@google.com/
---
Note: build-tested only.
Found with: $ rg "strncpy\("
---
drivers/net/mdio/mdio-gpio.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/mdio/mdio-gpio.c b/drivers/net/mdio/mdio-gpio.c
index 897b88c50bbb..778db310a28d 100644
--- a/drivers/net/mdio/mdio-gpio.c
+++ b/drivers/net/mdio/mdio-gpio.c
@@ -123,9 +123,9 @@ static struct mii_bus *mdio_gpio_bus_init(struct device
*dev,
new_bus->parent = dev;
if (bus_id != -1)
- snprintf(new_bus->id, MII_BUS_ID_SIZE, "gpio-%x", bus_id);
+ snprintf(new_bus->id, sizeof(new_bus->id), "gpio-%x", bus_id);
else
- strncpy(new_bus->id, "gpio", MII_BUS_ID_SIZE);
+ strscpy(new_bus->id, "gpio", sizeof(new_bus->id));
if (pdata) {
new_bus->phy_mask = pdata->phy_mask;
---
base-commit: bee0e7762ad2c6025b9f5245c040fcc36ef2bde8
change-id: 20231012-strncpy-drivers-net-mdio-mdio-gpio-c-bddd9ed0c630
Best regards,
--
Justin Stitt
Re: [PATCH v2 00/18] sysctl: constify sysctl ctl_tables
On 2023-12-08 10:59:26+0100, Joel Granados wrote:
> On Thu, Dec 07, 2023 at 08:19:43PM +0100, Thomas Weißschuh wrote:
> > On 2023-12-07 11:43:57+0100, Joel Granados wrote:
> [..]
> > > I suggest you chunk it up with directories in mind. Something similar to
> > > what I did for [4] where I divided stuff that when for fs/*, kernel/*,
> > > net/*, arch/* and drivers/*. That will complicate your patch a tad
> > > because you have to ensure that the tree can be compiled/run for every
> > > commit. But it will pay off once you push it to the broader public.
> >
> > This will break bisections. All function signatures need to be switched
> I was suggesting a solution without breaking bisections of course. I can
> think of a couple of ways to do this in chunks but it might be
> premature. You can send it and if you get push back because of this then
> we can deal with chunking it down.
I'm curious about those ways. I don't see how to split the big commit.
> I'm still concerned about the header size for those mails. How does the
> mail look like when you run the get maintainers script on it?
The full series has 142 recipients in total,
the biggest patch itself has 124.
Before sending it I'd like to get feedback on the internal rework of the
is_empty detection from you and/or Luis.
https://git.sr.ht/~t-8ch/linux/commit/ea27507070f3c47be6febebe451bbb88f6ea707e
or the attached patch.
> [..]
>From ea27507070f3c47be6febebe451bbb88f6ea707e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20Wei=C3=9Fschuh?=
Date: Sun, 3 Dec 2023 21:56:46 +0100
Subject: [PATCH] sysctl: move permanently empty flag to ctl_dir
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Simplify the logic by always keeping the permanently_empty flag on the
ctl_dir.
The previous logic kept the flag in the leaf ctl_table and from there
transferred it to the ctl_table from the directory.
This also removes the need to have a mutable ctl_table and will allow
the constification of those structs.
Signed-off-by: Thomas Weißschuh
---
fs/proc/proc_sysctl.c | 74 +++---
include/linux/sysctl.h | 13 ++--
2 files changed, 36 insertions(+), 51 deletions(-)
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 35c97ad54f34..33f41af58e9b 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include "internal.h"
#define list_for_each_table_entry(entry, header) \
@@ -29,32 +30,6 @@ static const struct inode_operations
proc_sys_inode_operations;
static const struct file_operations proc_sys_dir_file_operations;
static const struct inode_operations proc_sys_dir_operations;
-/* Support for permanently empty directories */
-static struct ctl_table sysctl_mount_point[] = {
- {.type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY }
-};
-
-/**
- * register_sysctl_mount_point() - registers a sysctl mount point
- * @path: path for the mount point
- *
- * Used to create a permanently empty directory to serve as mount point.
- * There are some subtle but important permission checks this allows in the
- * case of unprivileged mounts.
- */
-struct ctl_table_header *register_sysctl_mount_point(const char *path)
-{
- return register_sysctl(path, sysctl_mount_point);
-}
-EXPORT_SYMBOL(register_sysctl_mount_point);
-
-#define sysctl_is_perm_empty_ctl_header(hptr) \
- (hptr->ctl_table[0].type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
-#define sysctl_set_perm_empty_ctl_header(hptr) \
- (hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
-#define sysctl_clear_perm_empty_ctl_header(hptr) \
- (hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_DEFAULT)
-
void proc_sys_poll_notify(struct ctl_table_poll *poll)
{
if (!poll)
@@ -226,17 +201,9 @@ static int insert_header(struct ctl_dir *dir, struct
ctl_table_header *header)
/* Is this a permanently empty directory? */
- if (sysctl_is_perm_empty_ctl_header(dir_h))
+ if (dir->permanently_empty)
return -EROFS;
- /* Am I creating a permanently empty directory? */
- if (header->ctl_table_size > 0 &&
- sysctl_is_perm_empty_ctl_header(header)) {
- if (!RB_EMPTY_ROOT(&dir->root))
- return -EINVAL;
- sysctl_set_perm_empty_ctl_header(dir_h);
- }
-
dir_h->nreg++;
header->parent = dir;
err = insert_links(header);
@@ -252,8 +219,6 @@ fail:
erase_header(header);
put_links(header);
fail_links:
- if (header->ctl_table == sysctl_mount_point)
- sysctl_clear_perm_empty_ctl_header(dir_h);
header->parent = NULL;
drop_sysctl_table(dir_h);
return err;
@@ -440,6 +405,7 @@ static struct inode *proc_sys_make_inode(struct super_block
*sb,
struct ctl_table_header *head, struct ctl_table *table)
{
struc
RE: [PATCH v3] netlink: Return unsigned value for nla_len()
From: Kees Cook
> Sent: 06 December 2023 20:59
>
> The return value from nla_len() is never expected to be negative, and can
> never be more than struct nlattr::nla_len (a u16). Adjust the prototype
> on the function. This will let GCC's value range optimization passes
> know that the return can never be negative, and can never be larger than
> u16. As recently discussed[1], this silences the following warning in
> GCC 12+:
>
...
> -static inline int nla_len(const struct nlattr *nla)
> +static inline u16 nla_len(const struct nlattr *nla)
> {
> return nla->nla_len - NLA_HDRLEN;
> }
It also adds an explicit mask with 0x.
I suspect that returning 'unsigned int' will silence the warning
from gcc (since the error message has a huge max size).
If the value is too small copying ~64k or ~4G will both overflow the
buffer.
The former might (just) be exploitable, the latter will crash
(so is probably better!)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
Re: [PATCH v2 00/18] sysctl: constify sysctl ctl_tables
On Thu, Dec 07, 2023 at 08:19:43PM +0100, Thomas Weißschuh wrote:
> On 2023-12-07 11:43:57+0100, Joel Granados wrote:
> > Hey Thomas
> >
> > You have a couple of test bot issues for your 12/18 patch. Can you
> > please address those for your next version.
>
> I have these fixed locally, I assumed Luis would also pick them up
> directly until I have a proper v2, properly should have communicated
> that.
>
> > On Mon, Dec 04, 2023 at 08:52:13AM +0100, Thomas Weißschuh wrote:
> > > Problem description:
> > >
> > > The kernel contains a lot of struct ctl_table throught the tree.
> > > These are very often 'static' definitions.
> > > It would be good to make the tables unmodifiable by marking them "const"
> > Here I would remove "It would be good to". Just state it: "Make the
> > tables unmodifiable"
>
> Ack.
>
> >
> > > to avoid accidental or malicious modifications.
> > > This is in line with a general effort to move as much data as possible
> > > into .rodata. (See for example[0] and [1])
>
> > If you could find more examples, it would make a better case.
>
> I'll look for some. So far my constifications went in without them :-)
>
> > >
> > > Unfortunately the tables can not be made const right now because the
> > > core registration functions expect mutable tables.
> > >
> > > This is for two main reasons:
> > >
> > > 1) sysctl_{set,clear}_perm_empty_ctl_header in the sysctl core modify
> > >the table.
> > > 2) The table is passed to the handler function as a non-const pointer.
> > >
> > > This series migrates the core and all handlers.
>
> > awesome!
> >
> > >
> > > Structure of the series:
> > >
> > > Patch 1-3: Cleanup patches
> > > Patch 4-7: Non-logic preparation patches
> > > Patch 8: Preparation patch changing a bit of logic
> > > Patch 9-12: Treewide changes to handler function signature
> > > Patch 13-14: Adaption of the sysctl core implementation
> > > Patch 15:Adaption of the sysctl core interface
> > > Patch 16:New entry for checkpatch
> > > Patch 17-18: Constification of existing "struct ctl_table"s
> > >
> > > Tested by booting and with the sysctl selftests on x86.
> > >
> > > Note:
> > >
> > > This is intentionally sent only to a small number of people as I'd like
> > > to get some more sysctl core-maintainer feedback before sending this to
> > > essentially everybody.
>
> > When you do send it to the broader audience, you should chunk up your big
> > patches (12/18 and 11/18) and this is why:
> > 1. To avoid mail rejections from lists:
> >You have to tell a lot of people about the changes in one mail. That
> >will make mail header too big for some lists and it will be rejected.
> >This happened to me with [3]
> > 2. Avoid being rejected for the wrong reasons :)
> >Maintainers are busy ppl and sending them a set with so many files
> >may elicit a rejection on the grounds that it involves too many
> >subsystems at the same time.
> > I suggest you chunk it up with directories in mind. Something similar to
> > what I did for [4] where I divided stuff that when for fs/*, kernel/*,
> > net/*, arch/* and drivers/*. That will complicate your patch a tad
> > because you have to ensure that the tree can be compiled/run for every
> > commit. But it will pay off once you push it to the broader public.
>
> This will break bisections. All function signatures need to be switched
I was suggesting a solution without breaking bisections of course. I can
think of a couple of ways to do this in chunks but it might be
premature. You can send it and if you get push back because of this then
we can deal with chunking it down.
I'm still concerned about the header size for those mails. How does the
mail look like when you run the get maintainers script on it?
> in one step. I would strongly like to avoid introducing broken commits.
>
> The fact that these big commits have no functional changes at all makes
> me hope it can work.
--
Joel Granados
signature.asc
Description: PGP signature
