* [PATCH v4] iommu/vt-d: replace snprintf with scnprintf in dmar_latency_snapshot()
@ 2025-07-31 22:50 Seyediman Seyedarab
2025-08-07 9:00 ` Seyediman Seyedarab
0 siblings, 1 reply; 3+ messages in thread
From: Seyediman Seyedarab @ 2025-07-31 22:50 UTC (permalink / raw)
To: dwmw2, baolu.lu, joro, will, robin.murphy
Cc: iommu, linux-kernel, skhan, linux-kernel-mentees,
Seyediman Seyedarab
snprintf() returns the number of bytes that would have been
written, not the number actually written. Using this for offset
tracking can cause buffer overruns if truncation occurs.
Replace snprintf() with scnprintf() to ensure the offset stays
within bounds.
Since scnprintf() never returns a negative value, and zero is not
possible in this context because 'bytes' starts at 0 and 'size - bytes'
is DEBUG_BUFFER_SIZE in the first call, which is large enough to hold
the string literals used, the return value is always positive. An integer
overflow is also completely out of reach here due to the small and fixed
buffer size. The error check in latency_show_one() is therefore unnecessary.
Remove it and make dmar_latency_snapshot() return void.
Signed-off-by: Seyediman Seyedarab <ImanDevel@gmail.com>
---
Changes in v4:
- Removed 'ret' in latency_show_one() since it is not being used anymore:
https://lore.kernel.org/oe-kbuild-all/202508010632.WB0CM5Bz-lkp@intel.com/
Changes in v3:
- Restored return type of dmar_latency_enable() back to 'int'. It was
mistakenly changed to 'void' in the previous version.
Changes in v2:
- The return type of dmar_latency_snapshot() was changed based on the
discussion here:
https://lore.kernel.org/linux-iommu/aIDN3pvUSG3rN4SW@willie-the-truck/
drivers/iommu/intel/debugfs.c | 10 ++--------
drivers/iommu/intel/perf.c | 10 ++++------
drivers/iommu/intel/perf.h | 5 ++---
3 files changed, 8 insertions(+), 17 deletions(-)
diff --git a/drivers/iommu/intel/debugfs.c b/drivers/iommu/intel/debugfs.c
index affbf4a1558d..65d2f792f0f7 100644
--- a/drivers/iommu/intel/debugfs.c
+++ b/drivers/iommu/intel/debugfs.c
@@ -648,17 +648,11 @@ DEFINE_SHOW_ATTRIBUTE(ir_translation_struct);
static void latency_show_one(struct seq_file *m, struct intel_iommu *iommu,
struct dmar_drhd_unit *drhd)
{
- int ret;
-
seq_printf(m, "IOMMU: %s Register Base Address: %llx\n",
iommu->name, drhd->reg_base_addr);
- ret = dmar_latency_snapshot(iommu, debug_buf, DEBUG_BUFFER_SIZE);
- if (ret < 0)
- seq_puts(m, "Failed to get latency snapshot");
- else
- seq_puts(m, debug_buf);
- seq_puts(m, "\n");
+ dmar_latency_snapshot(iommu, debug_buf, DEBUG_BUFFER_SIZE);
+ seq_printf(m, "%s\n", debug_buf);
}
static int latency_show(struct seq_file *m, void *v)
diff --git a/drivers/iommu/intel/perf.c b/drivers/iommu/intel/perf.c
index adc4de6bbd88..dceeadc3ee7c 100644
--- a/drivers/iommu/intel/perf.c
+++ b/drivers/iommu/intel/perf.c
@@ -113,7 +113,7 @@ static char *latency_type_names[] = {
" svm_prq"
};
-int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
+void dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
{
struct latency_statistic *lstat = iommu->perf_statistic;
unsigned long flags;
@@ -122,7 +122,7 @@ int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
memset(str, 0, size);
for (i = 0; i < COUNTS_NUM; i++)
- bytes += snprintf(str + bytes, size - bytes,
+ bytes += scnprintf(str + bytes, size - bytes,
"%s", latency_counter_names[i]);
spin_lock_irqsave(&latency_lock, flags);
@@ -130,7 +130,7 @@ int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
if (!dmar_latency_enabled(iommu, i))
continue;
- bytes += snprintf(str + bytes, size - bytes,
+ bytes += scnprintf(str + bytes, size - bytes,
"\n%s", latency_type_names[i]);
for (j = 0; j < COUNTS_NUM; j++) {
@@ -156,11 +156,9 @@ int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
break;
}
- bytes += snprintf(str + bytes, size - bytes,
+ bytes += scnprintf(str + bytes, size - bytes,
"%12lld", val);
}
}
spin_unlock_irqrestore(&latency_lock, flags);
-
- return bytes;
}
diff --git a/drivers/iommu/intel/perf.h b/drivers/iommu/intel/perf.h
index df9a36942d64..1d4baad7e852 100644
--- a/drivers/iommu/intel/perf.h
+++ b/drivers/iommu/intel/perf.h
@@ -40,7 +40,7 @@ void dmar_latency_disable(struct intel_iommu *iommu, enum latency_type type);
bool dmar_latency_enabled(struct intel_iommu *iommu, enum latency_type type);
void dmar_latency_update(struct intel_iommu *iommu, enum latency_type type,
u64 latency);
-int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size);
+void dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size);
#else
static inline int
dmar_latency_enable(struct intel_iommu *iommu, enum latency_type type)
@@ -64,9 +64,8 @@ dmar_latency_update(struct intel_iommu *iommu, enum latency_type type, u64 laten
{
}
-static inline int
+static inline void
dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
{
- return 0;
}
#endif /* CONFIG_DMAR_PERF */
--
2.50.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v4] iommu/vt-d: replace snprintf with scnprintf in dmar_latency_snapshot()
2025-07-31 22:50 [PATCH v4] iommu/vt-d: replace snprintf with scnprintf in dmar_latency_snapshot() Seyediman Seyedarab
@ 2025-08-07 9:00 ` Seyediman Seyedarab
2025-08-08 5:19 ` Baolu Lu
0 siblings, 1 reply; 3+ messages in thread
From: Seyediman Seyedarab @ 2025-08-07 9:00 UTC (permalink / raw)
To: dwmw2, baolu.lu, joro, will, robin.murphy
Cc: iommu, linux-kernel, skhan, linux-kernel-mentees
On 25/07/31 06:50PM, Seyediman Seyedarab wrote:
> snprintf() returns the number of bytes that would have been
> written, not the number actually written. Using this for offset
> tracking can cause buffer overruns if truncation occurs.
>
> Replace snprintf() with scnprintf() to ensure the offset stays
> within bounds.
>
> Since scnprintf() never returns a negative value, and zero is not
> possible in this context because 'bytes' starts at 0 and 'size - bytes'
> is DEBUG_BUFFER_SIZE in the first call, which is large enough to hold
> the string literals used, the return value is always positive. An integer
> overflow is also completely out of reach here due to the small and fixed
> buffer size. The error check in latency_show_one() is therefore unnecessary.
> Remove it and make dmar_latency_snapshot() return void.
>
> Signed-off-by: Seyediman Seyedarab <ImanDevel@gmail.com>
> ---
> Changes in v4:
> - Removed 'ret' in latency_show_one() since it is not being used anymore:
> https://lore.kernel.org/oe-kbuild-all/202508010632.WB0CM5Bz-lkp@intel.com/
>
> Changes in v3:
> - Restored return type of dmar_latency_enable() back to 'int'. It was
> mistakenly changed to 'void' in the previous version.
>
> Changes in v2:
> - The return type of dmar_latency_snapshot() was changed based on the
> discussion here:
> https://lore.kernel.org/linux-iommu/aIDN3pvUSG3rN4SW@willie-the-truck/
>
>
> drivers/iommu/intel/debugfs.c | 10 ++--------
> drivers/iommu/intel/perf.c | 10 ++++------
> drivers/iommu/intel/perf.h | 5 ++---
> 3 files changed, 8 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/iommu/intel/debugfs.c b/drivers/iommu/intel/debugfs.c
> index affbf4a1558d..65d2f792f0f7 100644
> --- a/drivers/iommu/intel/debugfs.c
> +++ b/drivers/iommu/intel/debugfs.c
> @@ -648,17 +648,11 @@ DEFINE_SHOW_ATTRIBUTE(ir_translation_struct);
> static void latency_show_one(struct seq_file *m, struct intel_iommu *iommu,
> struct dmar_drhd_unit *drhd)
> {
> - int ret;
> -
> seq_printf(m, "IOMMU: %s Register Base Address: %llx\n",
> iommu->name, drhd->reg_base_addr);
>
> - ret = dmar_latency_snapshot(iommu, debug_buf, DEBUG_BUFFER_SIZE);
> - if (ret < 0)
> - seq_puts(m, "Failed to get latency snapshot");
> - else
> - seq_puts(m, debug_buf);
> - seq_puts(m, "\n");
> + dmar_latency_snapshot(iommu, debug_buf, DEBUG_BUFFER_SIZE);
> + seq_printf(m, "%s\n", debug_buf);
> }
>
> static int latency_show(struct seq_file *m, void *v)
> diff --git a/drivers/iommu/intel/perf.c b/drivers/iommu/intel/perf.c
> index adc4de6bbd88..dceeadc3ee7c 100644
> --- a/drivers/iommu/intel/perf.c
> +++ b/drivers/iommu/intel/perf.c
> @@ -113,7 +113,7 @@ static char *latency_type_names[] = {
> " svm_prq"
> };
>
> -int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
> +void dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
> {
> struct latency_statistic *lstat = iommu->perf_statistic;
> unsigned long flags;
> @@ -122,7 +122,7 @@ int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
> memset(str, 0, size);
>
> for (i = 0; i < COUNTS_NUM; i++)
> - bytes += snprintf(str + bytes, size - bytes,
> + bytes += scnprintf(str + bytes, size - bytes,
> "%s", latency_counter_names[i]);
>
> spin_lock_irqsave(&latency_lock, flags);
> @@ -130,7 +130,7 @@ int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
> if (!dmar_latency_enabled(iommu, i))
> continue;
>
> - bytes += snprintf(str + bytes, size - bytes,
> + bytes += scnprintf(str + bytes, size - bytes,
> "\n%s", latency_type_names[i]);
>
> for (j = 0; j < COUNTS_NUM; j++) {
> @@ -156,11 +156,9 @@ int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
> break;
> }
>
> - bytes += snprintf(str + bytes, size - bytes,
> + bytes += scnprintf(str + bytes, size - bytes,
> "%12lld", val);
> }
> }
> spin_unlock_irqrestore(&latency_lock, flags);
> -
> - return bytes;
> }
> diff --git a/drivers/iommu/intel/perf.h b/drivers/iommu/intel/perf.h
> index df9a36942d64..1d4baad7e852 100644
> --- a/drivers/iommu/intel/perf.h
> +++ b/drivers/iommu/intel/perf.h
> @@ -40,7 +40,7 @@ void dmar_latency_disable(struct intel_iommu *iommu, enum latency_type type);
> bool dmar_latency_enabled(struct intel_iommu *iommu, enum latency_type type);
> void dmar_latency_update(struct intel_iommu *iommu, enum latency_type type,
> u64 latency);
> -int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size);
> +void dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size);
> #else
> static inline int
> dmar_latency_enable(struct intel_iommu *iommu, enum latency_type type)
> @@ -64,9 +64,8 @@ dmar_latency_update(struct intel_iommu *iommu, enum latency_type type, u64 laten
> {
> }
>
> -static inline int
> +static inline void
> dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
> {
> - return 0;
> }
> #endif /* CONFIG_DMAR_PERF */
> --
> 2.50.1
>
Hi there,
Just following up on this patch. Please let me know if there's
anything else needed from my side.
Regards,
Seyediman
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v4] iommu/vt-d: replace snprintf with scnprintf in dmar_latency_snapshot()
2025-08-07 9:00 ` Seyediman Seyedarab
@ 2025-08-08 5:19 ` Baolu Lu
0 siblings, 0 replies; 3+ messages in thread
From: Baolu Lu @ 2025-08-08 5:19 UTC (permalink / raw)
To: Seyediman Seyedarab, dwmw2, joro, will, robin.murphy
Cc: iommu, linux-kernel, skhan, linux-kernel-mentees
On 8/7/25 17:00, Seyediman Seyedarab wrote:
> On 25/07/31 06:50PM, Seyediman Seyedarab wrote:
>> snprintf() returns the number of bytes that would have been
>> written, not the number actually written. Using this for offset
>> tracking can cause buffer overruns if truncation occurs.
>>
>> Replace snprintf() with scnprintf() to ensure the offset stays
>> within bounds.
>>
>> Since scnprintf() never returns a negative value, and zero is not
>> possible in this context because 'bytes' starts at 0 and 'size - bytes'
>> is DEBUG_BUFFER_SIZE in the first call, which is large enough to hold
>> the string literals used, the return value is always positive. An integer
>> overflow is also completely out of reach here due to the small and fixed
>> buffer size. The error check in latency_show_one() is therefore unnecessary.
>> Remove it and make dmar_latency_snapshot() return void.
>>
>> Signed-off-by: Seyediman Seyedarab<ImanDevel@gmail.com>
>> ---
>> Changes in v4:
>> - Removed 'ret' in latency_show_one() since it is not being used anymore:
>> https://lore.kernel.org/oe-kbuild-all/202508010632.WB0CM5Bz-lkp@intel.com/
>>
>> Changes in v3:
>> - Restored return type of dmar_latency_enable() back to 'int'. It was
>> mistakenly changed to 'void' in the previous version.
>>
>> Changes in v2:
>> - The return type of dmar_latency_snapshot() was changed based on the
>> discussion here:
>> https://lore.kernel.org/linux-iommu/aIDN3pvUSG3rN4SW@willie-the-truck/
>>
>>
>> drivers/iommu/intel/debugfs.c | 10 ++--------
>> drivers/iommu/intel/perf.c | 10 ++++------
>> drivers/iommu/intel/perf.h | 5 ++---
>> 3 files changed, 8 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/debugfs.c b/drivers/iommu/intel/debugfs.c
>> index affbf4a1558d..65d2f792f0f7 100644
>> --- a/drivers/iommu/intel/debugfs.c
>> +++ b/drivers/iommu/intel/debugfs.c
>> @@ -648,17 +648,11 @@ DEFINE_SHOW_ATTRIBUTE(ir_translation_struct);
>> static void latency_show_one(struct seq_file *m, struct intel_iommu *iommu,
>> struct dmar_drhd_unit *drhd)
>> {
>> - int ret;
>> -
>> seq_printf(m, "IOMMU: %s Register Base Address: %llx\n",
>> iommu->name, drhd->reg_base_addr);
>>
>> - ret = dmar_latency_snapshot(iommu, debug_buf, DEBUG_BUFFER_SIZE);
>> - if (ret < 0)
>> - seq_puts(m, "Failed to get latency snapshot");
>> - else
>> - seq_puts(m, debug_buf);
>> - seq_puts(m, "\n");
>> + dmar_latency_snapshot(iommu, debug_buf, DEBUG_BUFFER_SIZE);
>> + seq_printf(m, "%s\n", debug_buf);
>> }
>>
>> static int latency_show(struct seq_file *m, void *v)
>> diff --git a/drivers/iommu/intel/perf.c b/drivers/iommu/intel/perf.c
>> index adc4de6bbd88..dceeadc3ee7c 100644
>> --- a/drivers/iommu/intel/perf.c
>> +++ b/drivers/iommu/intel/perf.c
>> @@ -113,7 +113,7 @@ static char *latency_type_names[] = {
>> " svm_prq"
>> };
>>
>> -int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
>> +void dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
>> {
>> struct latency_statistic *lstat = iommu->perf_statistic;
>> unsigned long flags;
>> @@ -122,7 +122,7 @@ int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
>> memset(str, 0, size);
>>
>> for (i = 0; i < COUNTS_NUM; i++)
>> - bytes += snprintf(str + bytes, size - bytes,
>> + bytes += scnprintf(str + bytes, size - bytes,
>> "%s", latency_counter_names[i]);
>>
>> spin_lock_irqsave(&latency_lock, flags);
>> @@ -130,7 +130,7 @@ int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
>> if (!dmar_latency_enabled(iommu, i))
>> continue;
>>
>> - bytes += snprintf(str + bytes, size - bytes,
>> + bytes += scnprintf(str + bytes, size - bytes,
>> "\n%s", latency_type_names[i]);
>>
>> for (j = 0; j < COUNTS_NUM; j++) {
>> @@ -156,11 +156,9 @@ int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
>> break;
>> }
>>
>> - bytes += snprintf(str + bytes, size - bytes,
>> + bytes += scnprintf(str + bytes, size - bytes,
>> "%12lld", val);
>> }
>> }
>> spin_unlock_irqrestore(&latency_lock, flags);
>> -
>> - return bytes;
>> }
>> diff --git a/drivers/iommu/intel/perf.h b/drivers/iommu/intel/perf.h
>> index df9a36942d64..1d4baad7e852 100644
>> --- a/drivers/iommu/intel/perf.h
>> +++ b/drivers/iommu/intel/perf.h
>> @@ -40,7 +40,7 @@ void dmar_latency_disable(struct intel_iommu *iommu, enum latency_type type);
>> bool dmar_latency_enabled(struct intel_iommu *iommu, enum latency_type type);
>> void dmar_latency_update(struct intel_iommu *iommu, enum latency_type type,
>> u64 latency);
>> -int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size);
>> +void dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size);
>> #else
>> static inline int
>> dmar_latency_enable(struct intel_iommu *iommu, enum latency_type type)
>> @@ -64,9 +64,8 @@ dmar_latency_update(struct intel_iommu *iommu, enum latency_type type, u64 laten
>> {
>> }
>>
>> -static inline int
>> +static inline void
>> dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
>> {
>> - return 0;
>> }
>> #endif /* CONFIG_DMAR_PERF */
>> --
>> 2.50.1
>>
> Hi there,
>
> Just following up on this patch. Please let me know if there's
> anything else needed from my side.
It just missed the v6.17 merge window, I'll queue it for v6.18.
Thanks,
baolu
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-08-08 5:21 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-31 22:50 [PATCH v4] iommu/vt-d: replace snprintf with scnprintf in dmar_latency_snapshot() Seyediman Seyedarab
2025-08-07 9:00 ` Seyediman Seyedarab
2025-08-08 5:19 ` Baolu Lu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).