* [RFC 1/7] drm/amdkfd: avoid fragile and inefficient snprintf use
[not found] ` <1457469654-17059-1-git-send-email-linux@rasmusvillemoes.dk>
@ 2016-03-08 20:40 ` Rasmus Villemoes
2016-03-14 14:33 ` Oded Gabbay
0 siblings, 1 reply; 3+ messages in thread
From: Rasmus Villemoes @ 2016-03-08 20:40 UTC (permalink / raw)
To: Kees Cook, Andrew Morton, Oded Gabbay, David Airlie
Cc: Rasmus Villemoes, dri-devel, linux-kernel
Passing overlapping source and destination buffers to snprintf
formally has undefined behaviour and is rather fragile. While the
rather special case of passing the output buffer as the argument
corresponding to a leading "%s" in the format string currently works
with the kernel's printf implementation, that won't necessarily always
be the case (it's also needlessly inefficient, though that doesn't
matter much for sysfs files). Moreover, it might give the false
impression that other ways of overlapping source and destination
buffers would be ok.
The standard way of appending to a buffer with snprintf is to keep
track of the current string length (and thus also the remaining
available space). Using scnprintf ensures that the 'ret' variable will
always be strictly less than PAGE_SIZE, so we'll never pass a negative
buffer size to scnprintf, and we'll return the proper length to the
upper sysfs layer, whether truncation has happened or not.
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 168 +++++++++++++++---------------
1 file changed, 85 insertions(+), 83 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
index 74909e72a009..924cbf5e8db2 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
@@ -484,34 +484,34 @@ static int kfd_parse_crat_table(void *crat_image)
}
-#define sysfs_show_gen_prop(buffer, fmt, ...) \
- snprintf(buffer, PAGE_SIZE, "%s"fmt, buffer, __VA_ARGS__)
-#define sysfs_show_32bit_prop(buffer, name, value) \
- sysfs_show_gen_prop(buffer, "%s %u\n", name, value)
-#define sysfs_show_64bit_prop(buffer, name, value) \
- sysfs_show_gen_prop(buffer, "%s %llu\n", name, value)
-#define sysfs_show_32bit_val(buffer, value) \
- sysfs_show_gen_prop(buffer, "%u\n", value)
-#define sysfs_show_str_val(buffer, value) \
- sysfs_show_gen_prop(buffer, "%s\n", value)
+#define sysfs_show_gen_prop(buffer, len, fmt, ...) \
+ scnprintf(buffer + len, PAGE_SIZE - len, fmt, ##__VA_ARGS__)
+#define sysfs_show_32bit_prop(buffer, len, name, value) \
+ sysfs_show_gen_prop(buffer, len, "%s %u\n", name, value)
+#define sysfs_show_64bit_prop(buffer, len, name, value) \
+ sysfs_show_gen_prop(buffer, len, "%s %llu\n", name, value)
+#define sysfs_show_32bit_val(buffer, len, value) \
+ sysfs_show_gen_prop(buffer, len, "%u\n", value)
+#define sysfs_show_str_val(buffer, len, value) \
+ sysfs_show_gen_prop(buffer, len, "%s\n", value)
static ssize_t sysprops_show(struct kobject *kobj, struct attribute *attr,
char *buffer)
{
- ssize_t ret;
+ ssize_t ret = 0;
/* Making sure that the buffer is an empty string */
buffer[0] = 0;
if (attr == &sys_props.attr_genid) {
- ret = sysfs_show_32bit_val(buffer, sys_props.generation_count);
+ ret += sysfs_show_32bit_val(buffer, ret, sys_props.generation_count);
} else if (attr == &sys_props.attr_props) {
- sysfs_show_64bit_prop(buffer, "platform_oem",
- sys_props.platform_oem);
- sysfs_show_64bit_prop(buffer, "platform_id",
- sys_props.platform_id);
- ret = sysfs_show_64bit_prop(buffer, "platform_rev",
- sys_props.platform_rev);
+ ret += sysfs_show_64bit_prop(buffer, ret, "platform_oem",
+ sys_props.platform_oem);
+ ret += sysfs_show_64bit_prop(buffer, ret, "platform_id",
+ sys_props.platform_id);
+ ret += sysfs_show_64bit_prop(buffer, ret, "platform_rev",
+ sys_props.platform_rev);
} else {
ret = -EINVAL;
}
@@ -530,26 +530,26 @@ static struct kobj_type sysprops_type = {
static ssize_t iolink_show(struct kobject *kobj, struct attribute *attr,
char *buffer)
{
- ssize_t ret;
+ ssize_t ret = 0;
struct kfd_iolink_properties *iolink;
/* Making sure that the buffer is an empty string */
buffer[0] = 0;
iolink = container_of(attr, struct kfd_iolink_properties, attr);
- sysfs_show_32bit_prop(buffer, "type", iolink->iolink_type);
- sysfs_show_32bit_prop(buffer, "version_major", iolink->ver_maj);
- sysfs_show_32bit_prop(buffer, "version_minor", iolink->ver_min);
- sysfs_show_32bit_prop(buffer, "node_from", iolink->node_from);
- sysfs_show_32bit_prop(buffer, "node_to", iolink->node_to);
- sysfs_show_32bit_prop(buffer, "weight", iolink->weight);
- sysfs_show_32bit_prop(buffer, "min_latency", iolink->min_latency);
- sysfs_show_32bit_prop(buffer, "max_latency", iolink->max_latency);
- sysfs_show_32bit_prop(buffer, "min_bandwidth", iolink->min_bandwidth);
- sysfs_show_32bit_prop(buffer, "max_bandwidth", iolink->max_bandwidth);
- sysfs_show_32bit_prop(buffer, "recommended_transfer_size",
- iolink->rec_transfer_size);
- ret = sysfs_show_32bit_prop(buffer, "flags", iolink->flags);
+ ret += sysfs_show_32bit_prop(buffer, ret, "type", iolink->iolink_type);
+ ret += sysfs_show_32bit_prop(buffer, ret, "version_major", iolink->ver_maj);
+ ret += sysfs_show_32bit_prop(buffer, ret, "version_minor", iolink->ver_min);
+ ret += sysfs_show_32bit_prop(buffer, ret, "node_from", iolink->node_from);
+ ret += sysfs_show_32bit_prop(buffer, ret, "node_to", iolink->node_to);
+ ret += sysfs_show_32bit_prop(buffer, ret, "weight", iolink->weight);
+ ret += sysfs_show_32bit_prop(buffer, ret, "min_latency", iolink->min_latency);
+ ret += sysfs_show_32bit_prop(buffer, ret, "max_latency", iolink->max_latency);
+ ret += sysfs_show_32bit_prop(buffer, ret, "min_bandwidth", iolink->min_bandwidth);
+ ret += sysfs_show_32bit_prop(buffer, ret, "max_bandwidth", iolink->max_bandwidth);
+ ret += sysfs_show_32bit_prop(buffer, ret, "recommended_transfer_size",
+ iolink->rec_transfer_size);
+ ret += sysfs_show_32bit_prop(buffer, ret, "flags", iolink->flags);
return ret;
}
@@ -565,18 +565,18 @@ static struct kobj_type iolink_type = {
static ssize_t mem_show(struct kobject *kobj, struct attribute *attr,
char *buffer)
{
- ssize_t ret;
+ ssize_t ret = 0;
struct kfd_mem_properties *mem;
/* Making sure that the buffer is an empty string */
buffer[0] = 0;
mem = container_of(attr, struct kfd_mem_properties, attr);
- sysfs_show_32bit_prop(buffer, "heap_type", mem->heap_type);
- sysfs_show_64bit_prop(buffer, "size_in_bytes", mem->size_in_bytes);
- sysfs_show_32bit_prop(buffer, "flags", mem->flags);
- sysfs_show_32bit_prop(buffer, "width", mem->width);
- ret = sysfs_show_32bit_prop(buffer, "mem_clk_max", mem->mem_clk_max);
+ ret += sysfs_show_32bit_prop(buffer, ret, "heap_type", mem->heap_type);
+ ret += sysfs_show_64bit_prop(buffer, ret, "size_in_bytes", mem->size_in_bytes);
+ ret += sysfs_show_32bit_prop(buffer, ret, "flags", mem->flags);
+ ret += sysfs_show_32bit_prop(buffer, ret, "width", mem->width);
+ ret += sysfs_show_32bit_prop(buffer, ret, "mem_clk_max", mem->mem_clk_max);
return ret;
}
@@ -592,7 +592,7 @@ static struct kobj_type mem_type = {
static ssize_t kfd_cache_show(struct kobject *kobj, struct attribute *attr,
char *buffer)
{
- ssize_t ret;
+ ssize_t ret = 0;
uint32_t i;
struct kfd_cache_properties *cache;
@@ -600,20 +600,20 @@ static ssize_t kfd_cache_show(struct kobject *kobj, struct attribute *attr,
buffer[0] = 0;
cache = container_of(attr, struct kfd_cache_properties, attr);
- sysfs_show_32bit_prop(buffer, "processor_id_low",
- cache->processor_id_low);
- sysfs_show_32bit_prop(buffer, "level", cache->cache_level);
- sysfs_show_32bit_prop(buffer, "size", cache->cache_size);
- sysfs_show_32bit_prop(buffer, "cache_line_size", cache->cacheline_size);
- sysfs_show_32bit_prop(buffer, "cache_lines_per_tag",
- cache->cachelines_per_tag);
- sysfs_show_32bit_prop(buffer, "association", cache->cache_assoc);
- sysfs_show_32bit_prop(buffer, "latency", cache->cache_latency);
- sysfs_show_32bit_prop(buffer, "type", cache->cache_type);
- snprintf(buffer, PAGE_SIZE, "%ssibling_map ", buffer);
+ ret += sysfs_show_32bit_prop(buffer, ret, "processor_id_low",
+ cache->processor_id_low);
+ ret += sysfs_show_32bit_prop(buffer, ret, "level", cache->cache_level);
+ ret += sysfs_show_32bit_prop(buffer, ret, "size", cache->cache_size);
+ ret += sysfs_show_32bit_prop(buffer, ret, "cache_line_size", cache->cacheline_size);
+ ret += sysfs_show_32bit_prop(buffer, ret, "cache_lines_per_tag",
+ cache->cachelines_per_tag);
+ ret += sysfs_show_32bit_prop(buffer, ret, "association", cache->cache_assoc);
+ ret += sysfs_show_32bit_prop(buffer, ret, "latency", cache->cache_latency);
+ ret += sysfs_show_32bit_prop(buffer, ret, "type", cache->cache_type);
+ ret += scnprintf(buffer + ret, PAGE_SIZE - ret, "sibling_map ");
for (i = 0; i < KFD_TOPOLOGY_CPU_SIBLINGS; i++)
- ret = snprintf(buffer, PAGE_SIZE, "%s%d%s",
- buffer, cache->sibling_map[i],
+ ret += scnprintf(buffer + ret, PAGE_SIZE - ret, "%d%s",
+ cache->sibling_map[i],
(i == KFD_TOPOLOGY_CPU_SIBLINGS-1) ?
"\n" : ",");
@@ -631,6 +631,7 @@ static struct kobj_type cache_type = {
static ssize_t node_show(struct kobject *kobj, struct attribute *attr,
char *buffer)
{
+ ssize_t ret = 0;
struct kfd_topology_device *dev;
char public_name[KFD_TOPOLOGY_PUBLIC_NAME_SIZE];
uint32_t i;
@@ -642,7 +643,7 @@ static ssize_t node_show(struct kobject *kobj, struct attribute *attr,
if (strcmp(attr->name, "gpu_id") == 0) {
dev = container_of(attr, struct kfd_topology_device,
attr_gpuid);
- return sysfs_show_32bit_val(buffer, dev->gpu_id);
+ return sysfs_show_32bit_val(buffer, ret, dev->gpu_id);
}
if (strcmp(attr->name, "name") == 0) {
@@ -655,58 +656,58 @@ static ssize_t node_show(struct kobject *kobj, struct attribute *attr,
break;
}
public_name[KFD_TOPOLOGY_PUBLIC_NAME_SIZE-1] = 0x0;
- return sysfs_show_str_val(buffer, public_name);
+ return sysfs_show_str_val(buffer, ret, public_name);
}
dev = container_of(attr, struct kfd_topology_device,
attr_props);
- sysfs_show_32bit_prop(buffer, "cpu_cores_count",
- dev->node_props.cpu_cores_count);
- sysfs_show_32bit_prop(buffer, "simd_count",
- dev->node_props.simd_count);
+ ret += sysfs_show_32bit_prop(buffer, ret, "cpu_cores_count",
+ dev->node_props.cpu_cores_count);
+ ret += sysfs_show_32bit_prop(buffer, ret, "simd_count",
+ dev->node_props.simd_count);
if (dev->mem_bank_count < dev->node_props.mem_banks_count) {
pr_warn("kfd: mem_banks_count truncated from %d to %d\n",
dev->node_props.mem_banks_count,
dev->mem_bank_count);
- sysfs_show_32bit_prop(buffer, "mem_banks_count",
- dev->mem_bank_count);
+ ret += sysfs_show_32bit_prop(buffer, ret, "mem_banks_count",
+ dev->mem_bank_count);
} else {
- sysfs_show_32bit_prop(buffer, "mem_banks_count",
- dev->node_props.mem_banks_count);
+ ret += sysfs_show_32bit_prop(buffer, ret, "mem_banks_count",
+ dev->node_props.mem_banks_count);
}
- sysfs_show_32bit_prop(buffer, "caches_count",
+ ret += sysfs_show_32bit_prop(buffer, ret, "caches_count",
dev->node_props.caches_count);
- sysfs_show_32bit_prop(buffer, "io_links_count",
+ ret += sysfs_show_32bit_prop(buffer, ret, "io_links_count",
dev->node_props.io_links_count);
- sysfs_show_32bit_prop(buffer, "cpu_core_id_base",
+ ret += sysfs_show_32bit_prop(buffer, ret, "cpu_core_id_base",
dev->node_props.cpu_core_id_base);
- sysfs_show_32bit_prop(buffer, "simd_id_base",
+ ret += sysfs_show_32bit_prop(buffer, ret, "simd_id_base",
dev->node_props.simd_id_base);
- sysfs_show_32bit_prop(buffer, "max_waves_per_simd",
+ ret += sysfs_show_32bit_prop(buffer, ret, "max_waves_per_simd",
dev->node_props.max_waves_per_simd);
- sysfs_show_32bit_prop(buffer, "lds_size_in_kb",
+ ret += sysfs_show_32bit_prop(buffer, ret, "lds_size_in_kb",
dev->node_props.lds_size_in_kb);
- sysfs_show_32bit_prop(buffer, "gds_size_in_kb",
+ ret += sysfs_show_32bit_prop(buffer, ret, "gds_size_in_kb",
dev->node_props.gds_size_in_kb);
- sysfs_show_32bit_prop(buffer, "wave_front_size",
+ ret += sysfs_show_32bit_prop(buffer, ret, "wave_front_size",
dev->node_props.wave_front_size);
- sysfs_show_32bit_prop(buffer, "array_count",
+ ret += sysfs_show_32bit_prop(buffer, ret, "array_count",
dev->node_props.array_count);
- sysfs_show_32bit_prop(buffer, "simd_arrays_per_engine",
+ ret += sysfs_show_32bit_prop(buffer, ret, "simd_arrays_per_engine",
dev->node_props.simd_arrays_per_engine);
- sysfs_show_32bit_prop(buffer, "cu_per_simd_array",
+ ret += sysfs_show_32bit_prop(buffer, ret, "cu_per_simd_array",
dev->node_props.cu_per_simd_array);
- sysfs_show_32bit_prop(buffer, "simd_per_cu",
+ ret += sysfs_show_32bit_prop(buffer, ret, "simd_per_cu",
dev->node_props.simd_per_cu);
- sysfs_show_32bit_prop(buffer, "max_slots_scratch_cu",
+ ret += sysfs_show_32bit_prop(buffer, ret, "max_slots_scratch_cu",
dev->node_props.max_slots_scratch_cu);
- sysfs_show_32bit_prop(buffer, "vendor_id",
+ ret += sysfs_show_32bit_prop(buffer, ret, "vendor_id",
dev->node_props.vendor_id);
- sysfs_show_32bit_prop(buffer, "device_id",
+ ret += sysfs_show_32bit_prop(buffer, ret, "device_id",
dev->node_props.device_id);
- sysfs_show_32bit_prop(buffer, "location_id",
+ ret += sysfs_show_32bit_prop(buffer, ret, "location_id",
dev->node_props.location_id);
if (dev->gpu) {
@@ -723,23 +724,24 @@ static ssize_t node_show(struct kobject *kobj, struct attribute *attr,
HSA_CAP_WATCH_POINTS_TOTALBITS_MASK);
}
- sysfs_show_32bit_prop(buffer, "max_engine_clk_fcompute",
+ ret += sysfs_show_32bit_prop(buffer, ret, "max_engine_clk_fcompute",
dev->gpu->kfd2kgd->get_max_engine_clock_in_mhz(
dev->gpu->kgd));
- sysfs_show_64bit_prop(buffer, "local_mem_size",
+ ret += sysfs_show_64bit_prop(buffer, ret, "local_mem_size",
(unsigned long long int) 0);
- sysfs_show_32bit_prop(buffer, "fw_version",
+ ret += sysfs_show_32bit_prop(buffer, ret, "fw_version",
dev->gpu->kfd2kgd->get_fw_version(
dev->gpu->kgd,
KGD_ENGINE_MEC1));
- sysfs_show_32bit_prop(buffer, "capability",
+ ret += sysfs_show_32bit_prop(buffer, ret, "capability",
dev->node_props.capability);
}
- return sysfs_show_32bit_prop(buffer, "max_engine_clk_ccompute",
+ ret += sysfs_show_32bit_prop(buffer, ret, "max_engine_clk_ccompute",
cpufreq_quick_get_max(0)/1000);
+ return ret;
}
static const struct sysfs_ops node_ops = {
--
2.1.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [RFC 1/7] drm/amdkfd: avoid fragile and inefficient snprintf use
2016-03-08 20:40 ` [RFC 1/7] drm/amdkfd: avoid fragile and inefficient snprintf use Rasmus Villemoes
@ 2016-03-14 14:33 ` Oded Gabbay
2016-03-14 19:18 ` Rasmus Villemoes
0 siblings, 1 reply; 3+ messages in thread
From: Oded Gabbay @ 2016-03-14 14:33 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Andrew Morton, Kees Cook, Maling list - DRI developers,
Linux-Kernel@Vger. Kernel. Org
On Tue, Mar 8, 2016 at 10:40 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> Passing overlapping source and destination buffers to snprintf
> formally has undefined behaviour and is rather fragile. While the
> rather special case of passing the output buffer as the argument
> corresponding to a leading "%s" in the format string currently works
> with the kernel's printf implementation, that won't necessarily always
> be the case (it's also needlessly inefficient, though that doesn't
> matter much for sysfs files). Moreover, it might give the false
> impression that other ways of overlapping source and destination
> buffers would be ok.
>
> The standard way of appending to a buffer with snprintf is to keep
> track of the current string length (and thus also the remaining
> available space). Using scnprintf ensures that the 'ret' variable will
> always be strictly less than PAGE_SIZE, so we'll never pass a negative
> buffer size to scnprintf, and we'll return the proper length to the
> upper sysfs layer, whether truncation has happened or not.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
I saw there were some different opinions on this.
Have the fixes to the other drivers been taken ?
Oded
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC 1/7] drm/amdkfd: avoid fragile and inefficient snprintf use
2016-03-14 14:33 ` Oded Gabbay
@ 2016-03-14 19:18 ` Rasmus Villemoes
0 siblings, 0 replies; 3+ messages in thread
From: Rasmus Villemoes @ 2016-03-14 19:18 UTC (permalink / raw)
To: Oded Gabbay
Cc: Kees Cook, Andrew Morton, David Airlie,
Maling list - DRI developers, Linux-Kernel@Vger. Kernel. Org,
One Thousand Gnomes, Andy Shevchenko, Kalle Valo,
Greg Kroah-Hartman, Joe Perches
On Mon, Mar 14 2016, Oded Gabbay <oded.gabbay@gmail.com> wrote:
> On Tue, Mar 8, 2016 at 10:40 PM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>> Passing overlapping source and destination buffers to snprintf
>> formally has undefined behaviour and is rather fragile. While the
>
> I saw there were some different opinions on this.
> Have the fixes to the other drivers been taken ?
>
I rewrote this (as well as the joystick/analog.c and wlcore/boot.c
patches) to use seq_buf, and this patch in particular became much
simpler. But since akpm and Alan don't think there's anything to fix I'm
going to drop the series; if anyone wants to pursue this I'll be happy
to send them my v2.
Rasmus
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-03-14 19:18 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CAGXu5jJ-NApCpANjfz+bAEfwZJ8xizkM-jDHVhOPCzhxV-aqdA@mail.gmail.com>
[not found] ` <1457469654-17059-1-git-send-email-linux@rasmusvillemoes.dk>
2016-03-08 20:40 ` [RFC 1/7] drm/amdkfd: avoid fragile and inefficient snprintf use Rasmus Villemoes
2016-03-14 14:33 ` Oded Gabbay
2016-03-14 19:18 ` Rasmus Villemoes
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox