[PATCH v2] drm/amdkfd: Fix out-of-bounds read in kdf_create_vcrat_image_cpu()
KASAN reported a slab-out-of-bounds read of size 1 in
kdf_create_vcrat_image_cpu().
This occurs when, for example, when on an x86_64 with a single NUMA node
because kfd_fill_iolink_info_for_cpu() is a no-op, but afterwards the
sub_type_hdr->length, which is out-of-bounds, is read and multiplied by
entries. Fortunately, entries is 0 in this case so the overall
crat_table->length is still correct.
Check if there were any entries before de-referencing sub_type_hdr which
may be pointing to out-of-bounds memory.
Fixes: b7b6c38529c9 ("drm/amdkfd: Calculate CPU VCRAT size dynamically (v2)")
Suggested-by: Felix Kuehling
Signed-off-by: Jeremy Cline
---
drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
index 8cac497c2c45..a5640a6138cf 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
@@ -1040,11 +1040,14 @@ static int kfd_create_vcrat_image_cpu(void
*pcrat_image, size_t *size)
(struct crat_subtype_iolink *)sub_type_hdr);
if (ret < 0)
return ret;
- crat_table->length += (sub_type_hdr->length * entries);
- crat_table->total_entries += entries;
- sub_type_hdr = (typeof(sub_type_hdr))((char *)sub_type_hdr +
- sub_type_hdr->length * entries);
+ if (entries) {
+ crat_table->length += (sub_type_hdr->length * entries);
+ crat_table->total_entries += entries;
+
+ sub_type_hdr = (typeof(sub_type_hdr))((char
*)sub_type_hdr +
+ sub_type_hdr->length * entries);
+ }
#else
pr_info("IO link not available for non x86 platforms\n");
#endif
--
2.29.2
Re: [PATCH] drm/ttm: Fix address passed to dma_mapping_error() in ttm_pool_map()
On Mon, Jan 11, 2021 at 09:21:48PM +0100, Christian König wrote:
> Am 11.01.21 um 17:40 schrieb Jeremy Cline:
> > check_unmap() is producing a warning about a missing map error check.
> > The return value from dma_map_page() should be checked for an error, not
> > the caller-provided dma_addr.
> >
> > Fixes: d099fc8f540a ("drm/ttm: new TT backend allocation pool v3")
> > Signed-off-by: Jeremy Cline
>
> Good catch. Reviewed and pushed to drm-misc-fixes, should be in the -rc by
> the weekend.
>
Great, many thanks!
- Jeremy
[PATCH] drm/ttm: Fix address passed to dma_mapping_error() in ttm_pool_map()
check_unmap() is producing a warning about a missing map error check.
The return value from dma_map_page() should be checked for an error, not
the caller-provided dma_addr.
Fixes: d099fc8f540a ("drm/ttm: new TT backend allocation pool v3")
Signed-off-by: Jeremy Cline
---
drivers/gpu/drm/ttm/ttm_pool.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
index 7b2f60616750..0aa197204b08 100644
--- a/drivers/gpu/drm/ttm/ttm_pool.c
+++ b/drivers/gpu/drm/ttm/ttm_pool.c
@@ -190,7 +190,7 @@ static int ttm_pool_map(struct ttm_pool *pool, unsigned int
order,
size_t size = (1ULL << order) * PAGE_SIZE;
addr = dma_map_page(pool->dev, p, 0, size, DMA_BIDIRECTIONAL);
- if (dma_mapping_error(pool->dev, **dma_addr))
+ if (dma_mapping_error(pool->dev, addr))
return -EFAULT;
}
--
2.29.2
Re: [PATCH] amdgpu: Avoid sleeping during FPU critical sections
Hi,
On Mon, Jan 11, 2021 at 09:53:56AM +0100, Christian König wrote:
> Am 08.01.21 um 22:58 schrieb Jeremy Cline:
> > dcn20_resource_construct() includes a number of kzalloc(GFP_KERNEL)
> > calls which can sleep, but kernel_fpu_begin() disables preemption and
> > sleeping in this context is invalid.
> >
> > The only places the FPU appears to be required is in the
> > init_soc_bounding_box() function and when calculating the
> > {min,max}_fill_clk_mhz. Narrow the scope to just these two parts to
> > avoid sleeping while using the FPU.
> >
> > Fixes: 7a8a3430be15 ("amdgpu: Wrap FPU dependent functions in dc20")
> > Cc: Timothy Pearson
> > Signed-off-by: Jeremy Cline
>
> Good catch, but I would rather replace the kzalloc(GFP_KERNEL) with a
> kzalloc(GFP_ATOMIC) for now.
>
> We have tons of problems with this DC_FP_START()/DC_FP_END() annotations and
> are even in the process of moving them out of the file because the compiles
> tend to clutter FP registers even outside of the annotated ranges on some
> architectures.
>
Thanks for the review. Is it acceptable to move the DC_FP_END()
annotation up to the last usage? Keeping it where it is is probably
do-able, but covers things like calls to resource_construct() which
makes use of struct resource_create_funcs. I'm guessing only a sub-set
of the implementations are called via this function, but having an
interface which can't sleep sometimes doesn't sound appealing.
Happy to do it, but before I go down that road I just wanted to make
sure that's what you had in mind.
Thanks,
Jeremy
> > ---
> > drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c | 8
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
> > b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
> > index e04ecf0fc0db..a4fa5bf016c1 100644
> > --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
> > +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
> > @@ -3622,6 +3622,7 @@ static bool init_soc_bounding_box(struct dc *dc,
> > if (bb && ASICREV_IS_NAVI12_P(dc->ctx->asic_id.hw_internal_rev)) {
> > int i;
> > + DC_FP_START();
> > dcn2_0_nv12_soc.sr_exit_time_us =
> > fixed16_to_double_to_cpu(bb->sr_exit_time_us);
> > dcn2_0_nv12_soc.sr_enter_plus_exit_time_us =
> > @@ -3721,6 +3722,7 @@ static bool init_soc_bounding_box(struct dc *dc,
> > dcn2_0_nv12_soc.clock_limits[i].dram_speed_mts =
> >
> > fixed16_to_double_to_cpu(bb->clock_limits[i].dram_speed_mts);
> > }
> > + DC_FP_END();
> > }
> > if (pool->base.pp_smu) {
> > @@ -3777,8 +3779,6 @@ static bool dcn20_resource_construct(
> > enum dml_project dml_project_version =
> > get_dml_project_version(ctx->asic_id.hw_internal_rev);
> > - DC_FP_START();
> > -
> > ctx->dc_bios->regs = _regs;
> > pool->base.funcs = _res_pool_funcs;
> > @@ -3959,8 +3959,10 @@ static bool dcn20_resource_construct(
> > ranges.reader_wm_sets[i].wm_inst = i;
> > ranges.reader_wm_sets[i].min_drain_clk_mhz =
> > PP_SMU_WM_SET_RANGE_CLK_UNCONSTRAINED_MIN;
> > ranges.reader_wm_sets[i].max_drain_clk_mhz =
> > PP_SMU_WM_SET_RANGE_CLK_UNCONSTRAINED_MAX;
> > + DC_FP_START();
> > ranges.reader_wm_sets[i].min_fill_clk_mhz = (i
> > > 0) ? (loaded_bb->clock_limits[i - 1].dram_speed_mts / 16) + 1 : 0;
> > ranges.reader_wm_sets[i].max_fill_clk_mhz =
> > loaded_bb->clock_limits[i].dram_speed_mts / 16;
> > + DC_FP_END();
> > ranges.num_reader_wm_sets = i + 1;
> > }
> > @@ -4125,12 +4127,10 @@ static bool dcn20_resource_construct(
> > pool->base.oem_device = NULL;
> > }
> > - DC_FP_END();
> > return true;
> > create_fail:
> > - DC_FP_END();
> > dcn20_resource_destruct(pool);
> > return false;
>
Re: [PATCH] drm/amdkfd: Fix out-of-bounds read in kdf_create_vcrat_image_cpu()
On Fri, Jan 08, 2021 at 06:46:17PM -0500, Felix Kuehling wrote:
> Am 2021-01-08 um 11:31 a.m. schrieb Jeremy Cline:
> > KASAN reported a slab-out-of-bounds read of size 1 in
> > kdf_create_vcrat_image_cpu().
> >
> > This occurs when, for example, when on an x86_64 with a single NUMA node
> > because kfd_fill_iolink_info_for_cpu() is a no-op, but afterwards the
> > sub_type_hdr->length, which is out-of-bounds, is read and multiplied by
> > entries. Fortunately, entries is 0 in this case so the overall
> > crat_table->length is still correct.
>
> That's a pretty big change to fix that. Wouldn't it be enough to add a
> simple check after calling kfd_fill_iolink_info_for_cpu:
>
> if (entries) {
> crat_table->length += (sub_type_hdr->length * entries);
> crat_table->total_entries += entries;
> }
>
> Or change the output parameters of the kfd_fill_..._for_cpu functions
> from num_entries to size_filled, so the caller doesn't need to read
> sub_type_hdr->length any more.
>
For sure. I felt like this was a bit tidier afterwards, but that's an
opinion and not one I hold strongly. I'll look at preparing a smaller fix
next week.
Thanks,
Jeremy
> >
> > This refactors the helper functions to accept the crat_table directly
> > and calculate the table entry pointer based on the current table length.
> > This allows us to avoid an out-of-bounds read and hopefully makes the
> > pointer arithmetic clearer. It should have no functional change beyond
> > removing the out-of-bounds read.
> >
> > Fixes: b7b6c38529c9 ("drm/amdkfd: Calculate CPU VCRAT size dynamically
> > (v2)")
> > Signed-off-by: Jeremy Cline
> > ---
> > drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 86 +--
> > 1 file changed, 40 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> > b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> > index 8cac497c2c45..e50db2c0f4ee 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> > @@ -829,21 +829,24 @@ int kfd_create_crat_image_acpi(void **crat_image,
> > size_t *size)
> > /* kfd_fill_cu_for_cpu - Fill in Compute info for the given CPU NUMA node
> > *
> > * @numa_node_id: CPU NUMA node id
> > - * @avail_size: Available size in the memory
> > - * @sub_type_hdr: Memory into which compute info will be filled in
> > + * @avail_size: Available space in bytes at the end of the @crat_table.
> > + * @crat_table: The CRAT table to append the Compute info to;
> > + * on success the table length and total_entries count is updated.
> > *
> > * Return 0 if successful else return -ve value
> > */
> > static int kfd_fill_cu_for_cpu(int numa_node_id, int *avail_size,
> > - int proximity_domain,
> > - struct crat_subtype_computeunit *sub_type_hdr)
> > + struct crat_header *crat_table)
> > {
> > const struct cpumask *cpumask;
> > + struct crat_subtype_computeunit *sub_type_hdr;
> >
> > *avail_size -= sizeof(struct crat_subtype_computeunit);
> > if (*avail_size < 0)
> > return -ENOMEM;
> >
> > + sub_type_hdr = (typeof(sub_type_hdr))((char *)crat_table +
> > + crat_table->length);
> > memset(sub_type_hdr, 0, sizeof(struct crat_subtype_computeunit));
> >
> > /* Fill in subtype header data */
> > @@ -855,36 +858,42 @@ static int kfd_fill_cu_for_cpu(int numa_node_id, int
> > *avail_size,
> >
> > /* Fill in CU data */
> > sub_type_hdr->flags |= CRAT_CU_FLAGS_CPU_PRESENT;
> > - sub_type_hdr->proximity_domain = proximity_domain;
> > + sub_type_hdr->proximity_domain = crat_table->num_domains;
> > sub_type_hdr->processor_id_low = kfd_numa_node_to_apic_id(numa_node_id);
> > if (sub_type_hdr->processor_id_low == -1)
> > return -EINVAL;
> >
> > sub_type_hdr->num_cpu_cores = cpumask_weight(cpumask);
> >
> > + crat_table->length += sub_type_hdr->length;
> > + crat_table->total_entries++;
> > +
> > return 0;
> > }
> >
> > /* kfd_fill_mem_info_for_cpu - Fill in Memory info for the given CPU NUMA
> > node
> > *
> > * @numa_node_id: CPU NUMA node id
> > - * @avail_size: Available size in the memory
> > - * @sub_type_hdr: Memory into which compute info will be filled in
> > + * @avail_size: Available space in bytes at the end of
[PATCH] amdgpu: Avoid sleeping during FPU critical sections
dcn20_resource_construct() includes a number of kzalloc(GFP_KERNEL)
calls which can sleep, but kernel_fpu_begin() disables preemption and
sleeping in this context is invalid.
The only places the FPU appears to be required is in the
init_soc_bounding_box() function and when calculating the
{min,max}_fill_clk_mhz. Narrow the scope to just these two parts to
avoid sleeping while using the FPU.
Fixes: 7a8a3430be15 ("amdgpu: Wrap FPU dependent functions in dc20")
Cc: Timothy Pearson
Signed-off-by: Jeremy Cline
---
drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
index e04ecf0fc0db..a4fa5bf016c1 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
@@ -3622,6 +3622,7 @@ static bool init_soc_bounding_box(struct dc *dc,
if (bb && ASICREV_IS_NAVI12_P(dc->ctx->asic_id.hw_internal_rev)) {
int i;
+ DC_FP_START();
dcn2_0_nv12_soc.sr_exit_time_us =
fixed16_to_double_to_cpu(bb->sr_exit_time_us);
dcn2_0_nv12_soc.sr_enter_plus_exit_time_us =
@@ -3721,6 +3722,7 @@ static bool init_soc_bounding_box(struct dc *dc,
dcn2_0_nv12_soc.clock_limits[i].dram_speed_mts =
fixed16_to_double_to_cpu(bb->clock_limits[i].dram_speed_mts);
}
+ DC_FP_END();
}
if (pool->base.pp_smu) {
@@ -3777,8 +3779,6 @@ static bool dcn20_resource_construct(
enum dml_project dml_project_version =
get_dml_project_version(ctx->asic_id.hw_internal_rev);
- DC_FP_START();
-
ctx->dc_bios->regs = _regs;
pool->base.funcs = _res_pool_funcs;
@@ -3959,8 +3959,10 @@ static bool dcn20_resource_construct(
ranges.reader_wm_sets[i].wm_inst = i;
ranges.reader_wm_sets[i].min_drain_clk_mhz =
PP_SMU_WM_SET_RANGE_CLK_UNCONSTRAINED_MIN;
ranges.reader_wm_sets[i].max_drain_clk_mhz =
PP_SMU_WM_SET_RANGE_CLK_UNCONSTRAINED_MAX;
+ DC_FP_START();
ranges.reader_wm_sets[i].min_fill_clk_mhz = (i
> 0) ? (loaded_bb->clock_limits[i - 1].dram_speed_mts / 16) + 1 : 0;
ranges.reader_wm_sets[i].max_fill_clk_mhz =
loaded_bb->clock_limits[i].dram_speed_mts / 16;
+ DC_FP_END();
ranges.num_reader_wm_sets = i + 1;
}
@@ -4125,12 +4127,10 @@ static bool dcn20_resource_construct(
pool->base.oem_device = NULL;
}
- DC_FP_END();
return true;
create_fail:
- DC_FP_END();
dcn20_resource_destruct(pool);
return false;
--
2.28.0
[PATCH] drm/amdkfd: Fix out-of-bounds read in kdf_create_vcrat_image_cpu()
KASAN reported a slab-out-of-bounds read of size 1 in
kdf_create_vcrat_image_cpu().
This occurs when, for example, when on an x86_64 with a single NUMA node
because kfd_fill_iolink_info_for_cpu() is a no-op, but afterwards the
sub_type_hdr->length, which is out-of-bounds, is read and multiplied by
entries. Fortunately, entries is 0 in this case so the overall
crat_table->length is still correct.
This refactors the helper functions to accept the crat_table directly
and calculate the table entry pointer based on the current table length.
This allows us to avoid an out-of-bounds read and hopefully makes the
pointer arithmetic clearer. It should have no functional change beyond
removing the out-of-bounds read.
Fixes: b7b6c38529c9 ("drm/amdkfd: Calculate CPU VCRAT size dynamically (v2)")
Signed-off-by: Jeremy Cline
---
drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 86 +--
1 file changed, 40 insertions(+), 46 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
index 8cac497c2c45..e50db2c0f4ee 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
@@ -829,21 +829,24 @@ int kfd_create_crat_image_acpi(void **crat_image, size_t
*size)
/* kfd_fill_cu_for_cpu - Fill in Compute info for the given CPU NUMA node
*
* @numa_node_id: CPU NUMA node id
- * @avail_size: Available size in the memory
- * @sub_type_hdr: Memory into which compute info will be filled in
+ * @avail_size: Available space in bytes at the end of the @crat_table.
+ * @crat_table: The CRAT table to append the Compute info to;
+ * on success the table length and total_entries count is updated.
*
* Return 0 if successful else return -ve value
*/
static int kfd_fill_cu_for_cpu(int numa_node_id, int *avail_size,
- int proximity_domain,
- struct crat_subtype_computeunit *sub_type_hdr)
+ struct crat_header *crat_table)
{
const struct cpumask *cpumask;
+ struct crat_subtype_computeunit *sub_type_hdr;
*avail_size -= sizeof(struct crat_subtype_computeunit);
if (*avail_size < 0)
return -ENOMEM;
+ sub_type_hdr = (typeof(sub_type_hdr))((char *)crat_table +
+ crat_table->length);
memset(sub_type_hdr, 0, sizeof(struct crat_subtype_computeunit));
/* Fill in subtype header data */
@@ -855,36 +858,42 @@ static int kfd_fill_cu_for_cpu(int numa_node_id, int
*avail_size,
/* Fill in CU data */
sub_type_hdr->flags |= CRAT_CU_FLAGS_CPU_PRESENT;
- sub_type_hdr->proximity_domain = proximity_domain;
+ sub_type_hdr->proximity_domain = crat_table->num_domains;
sub_type_hdr->processor_id_low = kfd_numa_node_to_apic_id(numa_node_id);
if (sub_type_hdr->processor_id_low == -1)
return -EINVAL;
sub_type_hdr->num_cpu_cores = cpumask_weight(cpumask);
+ crat_table->length += sub_type_hdr->length;
+ crat_table->total_entries++;
+
return 0;
}
/* kfd_fill_mem_info_for_cpu - Fill in Memory info for the given CPU NUMA node
*
* @numa_node_id: CPU NUMA node id
- * @avail_size: Available size in the memory
- * @sub_type_hdr: Memory into which compute info will be filled in
+ * @avail_size: Available space in bytes at the end of the @crat_table.
+ * @crat_table: The CRAT table to append the Memory info to;
+ * on success the table length and total_entries count is updated.
*
* Return 0 if successful else return -ve value
*/
static int kfd_fill_mem_info_for_cpu(int numa_node_id, int *avail_size,
- int proximity_domain,
- struct crat_subtype_memory *sub_type_hdr)
+ struct crat_header *crat_table)
{
uint64_t mem_in_bytes = 0;
pg_data_t *pgdat;
int zone_type;
+ struct crat_subtype_memory *sub_type_hdr;
*avail_size -= sizeof(struct crat_subtype_memory);
if (*avail_size < 0)
return -ENOMEM;
+ sub_type_hdr = (typeof(sub_type_hdr))((char *)crat_table +
+ crat_table->length);
memset(sub_type_hdr, 0, sizeof(struct crat_subtype_memory));
/* Fill in subtype header data */
@@ -905,27 +914,37 @@ static int kfd_fill_mem_info_for_cpu(int numa_node_id,
int *avail_size,
sub_type_hdr->length_low = lower_32_bits(mem_in_bytes);
sub_type_hdr->length_high = upper_32_bits(mem_in_bytes);
- sub_type_hdr->proximity_domain = proximity_domain;
+ sub_type_hdr->proximity_domain = crat_table->num_domains;
+
+ crat_table->length += sub_type_hdr->length;
+ crat_table->total_entries++;
return 0;
}
#ifdef CONFIG_X86_64
+/* kfd_fill_iolink_info_for_
[PATCH] drm/nouveau: avoid a use-after-free when BO init fails
nouveau_bo_init() is backed by ttm_bo_init() and ferries its return code
back to the caller. On failures, ttm_bo_init() invokes the provided
destructor which should de-initialize and free the memory.
Thus, when nouveau_bo_init() returns an error the gem object has already
been released and the memory freed by nouveau_bo_del_ttm().
Fixes: 019cbd4a4feb ("drm/nouveau: Initialize GEM object before TTM object")
Cc: Thierry Reding
Signed-off-by: Jeremy Cline
---
drivers/gpu/drm/nouveau/nouveau_gem.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c
b/drivers/gpu/drm/nouveau/nouveau_gem.c
index 787d05eefd9c..d30157cc7169 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -211,10 +211,8 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size, int
align, uint32_t domain,
}
ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
- if (ret) {
- nouveau_bo_ref(NULL, );
+ if (ret)
return ret;
- }
/* we restrict allowed domains on nv50+ to only the types
* that were requested at creation time. not possibly on
--
2.28.0
[PATCH v2 3/3] drm/nouveau: clean up all clients on device removal
The postclose handler can run after the device has been removed (or the
driver has been unbound) since userspace clients are free to hold the
file open as long as they want. Because the device removal callback
frees the entire nouveau_drm structure, any reference to it in the
postclose handler will result in a use-after-free.
To reproduce this, one must simply open the device file, unbind the
driver (or physically remove the device), and then close the device
file. This was found and can be reproduced easily with the IGT
core_hotunplug tests.
To avoid this, all clients are cleaned up in the device finalization
rather than deferring it to the postclose handler, and the postclose
handler is protected by a critical section which ensures the
drm_dev_unplug() and the postclose handler won't race.
This is not an ideal fix, since as I understand the proposed plan for
the kernel<->userspace interface for hotplug support, destroying the
client before the file is closed will cause problems. However, I believe
to properly fix this issue, the lifetime of the nouveau_drm structure
needs to be extended to match the drm_device, and this proved to be a
rather invasive change. Thus, I've broken this out so the fix can be
easily backported.
Cc: sta...@vger.kernel.org
Signed-off-by: Jeremy Cline
---
drivers/gpu/drm/nouveau/nouveau_drm.c | 30 +++
1 file changed, 30 insertions(+)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 6ee1adc9bd40..afaf1774ee35 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -628,6 +628,7 @@ nouveau_drm_device_init(struct drm_device *dev)
static void
nouveau_drm_device_fini(struct drm_device *dev)
{
+ struct nouveau_cli *cli, *temp_cli;
struct nouveau_drm *drm = nouveau_drm(dev);
if (nouveau_pmops_runtime()) {
@@ -652,6 +653,24 @@ nouveau_drm_device_fini(struct drm_device *dev)
nouveau_ttm_fini(drm);
nouveau_vga_fini(drm);
+ /*
+* There may be existing clients from as-yet unclosed files. For now,
+* clean them up here rather than deferring until the file is closed,
+* but this likely not correct if we want to support hot-unplugging
+* properly.
+*/
+ mutex_lock(>clients_lock);
+ list_for_each_entry_safe(cli, temp_cli, >clients, head) {
+ list_del(>head);
+ mutex_lock(>mutex);
+ if (cli->abi16)
+ nouveau_abi16_fini(cli->abi16);
+ mutex_unlock(>mutex);
+ nouveau_cli_fini(cli);
+ kfree(cli);
+ }
+ mutex_unlock(>clients_lock);
+
nouveau_cli_fini(>client);
nouveau_cli_fini(>master);
nvif_parent_dtor(>parent);
@@ -,6 +1130,16 @@ nouveau_drm_postclose(struct drm_device *dev, struct
drm_file *fpriv)
{
struct nouveau_cli *cli = nouveau_cli(fpriv);
struct nouveau_drm *drm = nouveau_drm(dev);
+ int dev_index;
+
+ /*
+* The device is gone, and as it currently stands all clients are
+* cleaned up in the removal codepath. In the future this may change
+* so that we can support hot-unplugging, but for now we immediately
+* return to avoid a double-free situation.
+*/
+ if (!drm_dev_enter(dev, _index))
+ return;
pm_runtime_get_sync(dev->dev);
@@ -1127,6 +1156,7 @@ nouveau_drm_postclose(struct drm_device *dev, struct
drm_file *fpriv)
kfree(cli);
pm_runtime_mark_last_busy(dev->dev);
pm_runtime_put_autosuspend(dev->dev);
+ drm_dev_exit(dev_index);
}
static const struct drm_ioctl_desc
--
2.28.0
[PATCH v2 2/3] drm/nouveau: Add a dedicated mutex for the clients list
Rather than protecting the nouveau_drm clients list with the lock within
the "client" nouveau_cli, add a dedicated lock to serialize access to
the list. This is both clearer and necessary to avoid lockdep being
upset with us when we need to iterate through all the clients in the
list and potentially lock their mutex, which is the same class as the
lock protecting the entire list.
Cc: sta...@vger.kernel.org
Signed-off-by: Jeremy Cline
---
Changes since v1:
- Add a mutex_destroy() call when destroying the device struct
drivers/gpu/drm/nouveau/nouveau_drm.c | 10 ++
drivers/gpu/drm/nouveau/nouveau_drv.h | 5 +
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 4fe4d664c5f2..6ee1adc9bd40 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -557,6 +557,7 @@ nouveau_drm_device_init(struct drm_device *dev)
nvkm_dbgopt(nouveau_debug, "DRM");
INIT_LIST_HEAD(>clients);
+ mutex_init(>clients_lock);
spin_lock_init(>tile.lock);
/* workaround an odd issue on nvc1 by disabling the device's
@@ -654,6 +655,7 @@ nouveau_drm_device_fini(struct drm_device *dev)
nouveau_cli_fini(>client);
nouveau_cli_fini(>master);
nvif_parent_dtor(>parent);
+ mutex_destroy(>clients_lock);
kfree(drm);
}
@@ -1089,9 +1091,9 @@ nouveau_drm_open(struct drm_device *dev, struct drm_file
*fpriv)
fpriv->driver_priv = cli;
- mutex_lock(>client.mutex);
+ mutex_lock(>clients_lock);
list_add(>head, >clients);
- mutex_unlock(>client.mutex);
+ mutex_unlock(>clients_lock);
done:
if (ret && cli) {
@@ -1117,9 +1119,9 @@ nouveau_drm_postclose(struct drm_device *dev, struct
drm_file *fpriv)
nouveau_abi16_fini(cli->abi16);
mutex_unlock(>mutex);
- mutex_lock(>client.mutex);
+ mutex_lock(>clients_lock);
list_del(>head);
- mutex_unlock(>client.mutex);
+ mutex_unlock(>clients_lock);
nouveau_cli_fini(cli);
kfree(cli);
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
b/drivers/gpu/drm/nouveau/nouveau_drv.h
index 9d04d1b36434..550e5f335146 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -141,6 +141,11 @@ struct nouveau_drm {
struct list_head clients;
+ /**
+* @clients_lock: Protects access to the @clients list of
nouveau_cli.
+*/
+ struct mutex clients_lock;
+
u8 old_pm_cap;
struct {
--
2.28.0
[PATCH v2 1/3] drm/nouveau: use drm_dev_unplug() during device removal
Nouveau does not currently support hot-unplugging, but it still makes sense to switch from drm_dev_unregister() to drm_dev_unplug(). drm_dev_unplug() calls drm_dev_unregister() after marking the device as unplugged, but only after any device critical sections are finished. Since nouveau isn't using drm_dev_enter() and drm_dev_exit(), there are no critical sections so this is nearly functionally equivalent. However, the DRM layer does check to see if the device is unplugged, and if it is returns appropriate error codes. In the future nouveau can add critical sections in order to truly support hot-unplugging. Cc: sta...@vger.kernel.org Signed-off-by: Jeremy Cline --- drivers/gpu/drm/nouveau/nouveau_drm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index d141a5f004af..4fe4d664c5f2 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -792,7 +792,7 @@ nouveau_drm_device_remove(struct drm_device *dev) struct nvkm_client *client; struct nvkm_device *device; - drm_dev_unregister(dev); + drm_dev_unplug(dev); dev->irq_enabled = false; client = nvxx_client(>client.base); -- 2.28.0
[PATCH v2 0/3] drm/nouveau: fix a use-after-free in postclose()
This series fixes a number of use-after-frees in nouveau's postclose() handler. It was discovered by pointing IGT's core_hotunplug tests at a nouveau device, but the steps to reproduce it are simple: 1. Open the device file 2. Unbind the driver or remove the device 3. Close the file opened in step 1. During the device removal, the nouveau_drm structure is de-allocated, but is dereferenced in the postclose() handler. One obvious solution is to ensure all the operations in the postclose() handler are valid by extending the lifetime of the nouveau_drm structure. This is possible with the new devm_drm_dev_alloc() interface, but the change is somewhat invasive so I thought it best to submit that work separately. Instead, we make use of the drm_dev_unplug() API, clean up all clients in the device removal call, and check to make sure the device has not been unplugged in the postclose() handler. While this does not enable hot-unplug support for nouveau, it's enough to avoid crashing the kernel and leads to all the core_hotunplug tests to pass. This series reroll addresses a missing mutex_destroy() call and a typo in a commit message. Jeremy Cline (3): drm/nouveau: use drm_dev_unplug() during device removal drm/nouveau: Add a dedicated mutex for the clients list drm/nouveau: clean up all clients on device removal drivers/gpu/drm/nouveau/nouveau_drm.c | 42 +++ drivers/gpu/drm/nouveau/nouveau_drv.h | 5 2 files changed, 42 insertions(+), 5 deletions(-) -- 2.28.0
Re: [PATCH 2/3] drm/nouveau: Add a dedicated mutex for the clients list
On Wed, Nov 25, 2020 at 01:37:06PM -0500, Lyude Paul wrote:
> On Tue, 2020-11-03 at 14:49 -0500, Jeremy Cline wrote:
> > Rather than protecting the nouveau_drm clients list with the lock within
> > the "client" nouveau_cli, add a dedicated lock to serialize access to
> > the list. This is both clearer and necessary to avoid lockdep being
> > upset with us when we need to iterate through all the clients in the
> > list and potentially lock their mutex, which is the same class as the
> > lock protecting the entire list.
> >
> > Signed-off-by: Jeremy Cline
> > ---
> > drivers/gpu/drm/nouveau/nouveau_drm.c | 9 +
> > drivers/gpu/drm/nouveau/nouveau_drv.h | 5 +
> > 2 files changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > index 4fe4d664c5f2..d182b877258a 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > @@ -557,6 +557,7 @@ nouveau_drm_device_init(struct drm_device *dev)
> > nvkm_dbgopt(nouveau_debug, "DRM");
> >
> > INIT_LIST_HEAD(>clients);
> > + mutex_init(>clients_lock);
>
> Looks like you forgot to hook up mutex_destroy() somewhere. Note there's
> actually plenty of code in nouveau right now that forgets to do this, but at
> some point we should probably fix that and avoid adding more spots where
> there's
> no mutex_destroy().
>
I'm guilty of having looked at the existing locking init code in nouveau
and modeling this work accordingly. I'll send out a fix for this shortly
and look at tidying up the rest of the locks in a separate series.
Thanks!
> > spin_lock_init(>tile.lock);
> >
> > /* workaround an odd issue on nvc1 by disabling the device's
> > @@ -1089,9 +1090,9 @@ nouveau_drm_open(struct drm_device *dev, struct
> > drm_file
> > *fpriv)
> >
> > fpriv->driver_priv = cli;
> >
> > - mutex_lock(>client.mutex);
> > + mutex_lock(>clients_lock);
> > list_add(>head, >clients);
> > - mutex_unlock(>client.mutex);
> > + mutex_unlock(>clients_lock);
> >
> > done:
> > if (ret && cli) {
> > @@ -1117,9 +1118,9 @@ nouveau_drm_postclose(struct drm_device *dev, struct
> > drm_file *fpriv)
> > nouveau_abi16_fini(cli->abi16);
> > mutex_unlock(>mutex);
> >
> > - mutex_lock(>client.mutex);
> > + mutex_lock(>clients_lock);
> > list_del(>head);
> > - mutex_unlock(>client.mutex);
> > + mutex_unlock(>clients_lock);
> >
> > nouveau_cli_fini(cli);
> > kfree(cli);
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
> > b/drivers/gpu/drm/nouveau/nouveau_drv.h
> > index 9d04d1b36434..550e5f335146 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> > @@ -141,6 +141,11 @@ struct nouveau_drm {
> >
> > struct list_head clients;
> >
> > + /**
> > + * @clients_lock: Protects access to the @clients list of
> > nouveau_cli.
> > + */
> > + struct mutex clients_lock;
> > +
> > u8 old_pm_cap;
> >
> > struct {
>
> --
> Sincerely,
>Lyude Paul (she/her)
>Software Engineer at Red Hat
>
> Note: I deal with a lot of emails and have a lot of bugs on my plate. If
> you've
> asked me a question, are waiting for a review/merge on a patch, etc. and I
> haven't responded in a while, please feel free to send me another email to
> check
> on my status. I don't bite!
>
Re: [Nouveau] [PATCH 2/3] drm/nouveau: manage nouveau_drm lifetime with devres
On Fri, Nov 06, 2020 at 02:31:44PM +0100, Karol Herbst wrote:
> On Fri, Nov 6, 2020 at 3:17 AM Jeremy Cline wrote:
> >
> > Make use of the devm_drm_dev_alloc() API to bind the lifetime of
> > nouveau_drm structure to the drm_device. This is important because a
> > reference to nouveau_drm is accessible from drm_device, which is
> > provided to a number of DRM layer callbacks that can run after the
> > deallocation of nouveau_drm currently occurs.
> >
> > Signed-off-by: Jeremy Cline
> > ---
> > drivers/gpu/drm/nouveau/nouveau_drm.c | 44 ---
> > drivers/gpu/drm/nouveau/nouveau_drv.h | 10 --
> > 2 files changed, 26 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > index bc6f51bf23b7..f750c25e92f9 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > @@ -30,9 +30,11 @@
> > #include
> > #include
> >
> > +#include
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -532,13 +534,8 @@ nouveau_parent = {
> > static int
> > nouveau_drm_device_init(struct drm_device *dev)
> > {
> > - struct nouveau_drm *drm;
> > int ret;
> > -
> > - if (!(drm = kzalloc(sizeof(*drm), GFP_KERNEL)))
> > - return -ENOMEM;
> > - dev->dev_private = drm;
> > - drm->dev = dev;
> > + struct nouveau_drm *drm = nouveau_drm(dev);
> >
> > nvif_parent_ctor(_parent, >parent);
> > drm->master.base.object.parent = >parent;
> > @@ -620,7 +617,6 @@ nouveau_drm_device_init(struct drm_device *dev)
> > nouveau_cli_fini(>master);
> > fail_alloc:
> > nvif_parent_dtor(>parent);
> > - kfree(drm);
> > return ret;
> > }
> >
> > @@ -654,7 +650,6 @@ nouveau_drm_device_fini(struct drm_device *dev)
> > nouveau_cli_fini(>client);
> > nouveau_cli_fini(>master);
> > nvif_parent_dtor(>parent);
> > - kfree(drm);
> > }
> >
> > /*
> > @@ -720,6 +715,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> > {
> > struct nvkm_device *device;
> > struct drm_device *drm_dev;
> > + struct nouveau_drm *nv_dev;
> > int ret;
> >
> > if (vga_switcheroo_client_probe_defer(pdev))
> > @@ -750,15 +746,16 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> > if (nouveau_atomic)
> > driver_pci.driver_features |= DRIVER_ATOMIC;
> >
> > - drm_dev = drm_dev_alloc(_pci, >dev);
> > - if (IS_ERR(drm_dev)) {
> > - ret = PTR_ERR(drm_dev);
> > + nv_dev = devm_drm_dev_alloc(>dev, _stub,
> > typeof(*nv_dev), drm_dev);
> > + if (IS_ERR(nv_dev)) {
> > + ret = PTR_ERR(nv_dev);
> > goto fail_nvkm;
> > }
> > + drm_dev = nouveau_to_drm_dev(nv_dev);
> >
> > ret = pci_enable_device(pdev);
> > if (ret)
> > - goto fail_drm;
> > + goto fail_nvkm;
> >
> > drm_dev->pdev = pdev;
> > pci_set_drvdata(pdev, drm_dev);
> > @@ -778,8 +775,6 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> > nouveau_drm_device_fini(drm_dev);
> > fail_pci:
> > pci_disable_device(pdev);
> > -fail_drm:
> > - drm_dev_put(drm_dev);
>
> it sounded like that when using devm_drm_dev_alloc we still have an
> initial refcount of 1, so at least in this regard nothing changed so I
> am wondering why this change is necessary and if the reason is
> unrelated it might make sense to move it into its own patch.
>
Okay, after a more thorough investigation and testing with
drm.debug=0x201 to trace through the allocations and de-allocations,
everything is indeed cleaned up, both here when a fake failure is
injected, and through the normal removal path.
I believe it would be problematic if nouveau was presented the device,
called devm_drm_dev_alloc(), failed later on in the probe, and then a
different driver claimed the device. It looks like that's not a problem
in practice, but I'm not familiar enough with all the layers to be 100%
confident I'm reading everything right. As far as I can tell, amdgpu
isn't explicitly dropping the reference either.
> > fail_nvkm:
> > nvkm_device_del();
> >
Re: [Nouveau] [PATCH 2/3] drm/nouveau: manage nouveau_drm lifetime with devres
On Fri, Nov 06, 2020 at 02:31:44PM +0100, Karol Herbst wrote:
> On Fri, Nov 6, 2020 at 3:17 AM Jeremy Cline wrote:
> >
> > Make use of the devm_drm_dev_alloc() API to bind the lifetime of
> > nouveau_drm structure to the drm_device. This is important because a
> > reference to nouveau_drm is accessible from drm_device, which is
> > provided to a number of DRM layer callbacks that can run after the
> > deallocation of nouveau_drm currently occurs.
> >
> > Signed-off-by: Jeremy Cline
> > ---
> > drivers/gpu/drm/nouveau/nouveau_drm.c | 44 ---
> > drivers/gpu/drm/nouveau/nouveau_drv.h | 10 --
> > 2 files changed, 26 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > index bc6f51bf23b7..f750c25e92f9 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > @@ -30,9 +30,11 @@
> > #include
> > #include
> >
> > +#include
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -532,13 +534,8 @@ nouveau_parent = {
> > static int
> > nouveau_drm_device_init(struct drm_device *dev)
> > {
> > - struct nouveau_drm *drm;
> > int ret;
> > -
> > - if (!(drm = kzalloc(sizeof(*drm), GFP_KERNEL)))
> > - return -ENOMEM;
> > - dev->dev_private = drm;
> > - drm->dev = dev;
> > + struct nouveau_drm *drm = nouveau_drm(dev);
> >
> > nvif_parent_ctor(_parent, >parent);
> > drm->master.base.object.parent = >parent;
> > @@ -620,7 +617,6 @@ nouveau_drm_device_init(struct drm_device *dev)
> > nouveau_cli_fini(>master);
> > fail_alloc:
> > nvif_parent_dtor(>parent);
> > - kfree(drm);
> > return ret;
> > }
> >
> > @@ -654,7 +650,6 @@ nouveau_drm_device_fini(struct drm_device *dev)
> > nouveau_cli_fini(>client);
> > nouveau_cli_fini(>master);
> > nvif_parent_dtor(>parent);
> > - kfree(drm);
> > }
> >
> > /*
> > @@ -720,6 +715,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> > {
> > struct nvkm_device *device;
> > struct drm_device *drm_dev;
> > + struct nouveau_drm *nv_dev;
> > int ret;
> >
> > if (vga_switcheroo_client_probe_defer(pdev))
> > @@ -750,15 +746,16 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> > if (nouveau_atomic)
> > driver_pci.driver_features |= DRIVER_ATOMIC;
> >
> > - drm_dev = drm_dev_alloc(_pci, >dev);
> > - if (IS_ERR(drm_dev)) {
> > - ret = PTR_ERR(drm_dev);
> > + nv_dev = devm_drm_dev_alloc(>dev, _stub,
> > typeof(*nv_dev), drm_dev);
> > + if (IS_ERR(nv_dev)) {
> > + ret = PTR_ERR(nv_dev);
> > goto fail_nvkm;
> > }
> > + drm_dev = nouveau_to_drm_dev(nv_dev);
> >
> > ret = pci_enable_device(pdev);
> > if (ret)
> > - goto fail_drm;
> > + goto fail_nvkm;
> >
> > drm_dev->pdev = pdev;
> > pci_set_drvdata(pdev, drm_dev);
> > @@ -778,8 +775,6 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> > nouveau_drm_device_fini(drm_dev);
> > fail_pci:
> > pci_disable_device(pdev);
> > -fail_drm:
> > - drm_dev_put(drm_dev);
>
> it sounded like that when using devm_drm_dev_alloc we still have an
> initial refcount of 1, so at least in this regard nothing changed so I
> am wondering why this change is necessary and if the reason is
> unrelated it might make sense to move it into its own patch.
>
The way I read the supporting code is that when the allocation occurs,
an action is registered on the parent device to call drm_dev_put(), so
as long as the PCI device is dropped, and as far as I could tell it is
when an error is returned, it should be handled automatically. The same
I *think* goes for the platform device variety with Tegra.
However, this is by far the most likely thing for me to have
misunderstood so I'll look through it a second time and would love to
have a second opinion on it.
> > fail_nvkm:
> > nvkm_device_del();
> > return ret;
> > @@ -799,7 +794,6 @@ nouveau_drm_device_remove(struct drm_device *dev)
> > device = nvkm_device_find(clien
[PATCH 2/3] drm/nouveau: manage nouveau_drm lifetime with devres
Make use of the devm_drm_dev_alloc() API to bind the lifetime of
nouveau_drm structure to the drm_device. This is important because a
reference to nouveau_drm is accessible from drm_device, which is
provided to a number of DRM layer callbacks that can run after the
deallocation of nouveau_drm currently occurs.
Signed-off-by: Jeremy Cline
---
drivers/gpu/drm/nouveau/nouveau_drm.c | 44 ---
drivers/gpu/drm/nouveau/nouveau_drv.h | 10 --
2 files changed, 26 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index bc6f51bf23b7..f750c25e92f9 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -30,9 +30,11 @@
#include
#include
+#include
#include
#include
#include
+#include
#include
#include
@@ -532,13 +534,8 @@ nouveau_parent = {
static int
nouveau_drm_device_init(struct drm_device *dev)
{
- struct nouveau_drm *drm;
int ret;
-
- if (!(drm = kzalloc(sizeof(*drm), GFP_KERNEL)))
- return -ENOMEM;
- dev->dev_private = drm;
- drm->dev = dev;
+ struct nouveau_drm *drm = nouveau_drm(dev);
nvif_parent_ctor(_parent, >parent);
drm->master.base.object.parent = >parent;
@@ -620,7 +617,6 @@ nouveau_drm_device_init(struct drm_device *dev)
nouveau_cli_fini(>master);
fail_alloc:
nvif_parent_dtor(>parent);
- kfree(drm);
return ret;
}
@@ -654,7 +650,6 @@ nouveau_drm_device_fini(struct drm_device *dev)
nouveau_cli_fini(>client);
nouveau_cli_fini(>master);
nvif_parent_dtor(>parent);
- kfree(drm);
}
/*
@@ -720,6 +715,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
{
struct nvkm_device *device;
struct drm_device *drm_dev;
+ struct nouveau_drm *nv_dev;
int ret;
if (vga_switcheroo_client_probe_defer(pdev))
@@ -750,15 +746,16 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
if (nouveau_atomic)
driver_pci.driver_features |= DRIVER_ATOMIC;
- drm_dev = drm_dev_alloc(_pci, >dev);
- if (IS_ERR(drm_dev)) {
- ret = PTR_ERR(drm_dev);
+ nv_dev = devm_drm_dev_alloc(>dev, _stub, typeof(*nv_dev),
drm_dev);
+ if (IS_ERR(nv_dev)) {
+ ret = PTR_ERR(nv_dev);
goto fail_nvkm;
}
+ drm_dev = nouveau_to_drm_dev(nv_dev);
ret = pci_enable_device(pdev);
if (ret)
- goto fail_drm;
+ goto fail_nvkm;
drm_dev->pdev = pdev;
pci_set_drvdata(pdev, drm_dev);
@@ -778,8 +775,6 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
nouveau_drm_device_fini(drm_dev);
fail_pci:
pci_disable_device(pdev);
-fail_drm:
- drm_dev_put(drm_dev);
fail_nvkm:
nvkm_device_del();
return ret;
@@ -799,7 +794,6 @@ nouveau_drm_device_remove(struct drm_device *dev)
device = nvkm_device_find(client->device);
nouveau_drm_device_fini(dev);
- drm_dev_put(dev);
nvkm_device_del();
}
@@ -1285,7 +1279,8 @@ nouveau_platform_device_create(const struct
nvkm_device_tegra_func *func,
struct platform_device *pdev,
struct nvkm_device **pdevice)
{
- struct drm_device *drm;
+ struct nouveau_drm *nv_dev;
+ struct drm_device *drm_dev;
int err;
err = nvkm_device_tegra_new(func, pdev, nouveau_config, nouveau_debug,
@@ -1293,22 +1288,21 @@ nouveau_platform_device_create(const struct
nvkm_device_tegra_func *func,
if (err)
goto err_free;
- drm = drm_dev_alloc(_platform, >dev);
- if (IS_ERR(drm)) {
- err = PTR_ERR(drm);
+ nv_dev = devm_drm_dev_alloc(>dev, _platform,
typeof(*nv_dev), drm_dev);
+ if (IS_ERR(nv_dev)) {
+ err = PTR_ERR(nv_dev);
goto err_free;
}
+ drm_dev = nouveau_to_drm_dev(nv_dev);
- err = nouveau_drm_device_init(drm);
+ err = nouveau_drm_device_init(drm_dev);
if (err)
- goto err_put;
+ goto err_free;
- platform_set_drvdata(pdev, drm);
+ platform_set_drvdata(pdev, drm_dev);
- return drm;
+ return drm_dev;
-err_put:
- drm_dev_put(drm);
err_free:
nvkm_device_del(pdevice);
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
b/drivers/gpu/drm/nouveau/nouveau_drv.h
index 3e2920a10099..cf6c33e52a5c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -137,7 +137,11 @@ struct nouveau_drm {
struct nvif_parent parent;
struct nouveau_cli master;
struct nouveau_cli client;
- struct drm_device *dev;
+
+ /**
+* @drm_dev: The parent DRM device object.
+*/
+ struc
[PATCH 1/3] drm/nouveau: Use helper to convert nouveau_drm to drm_device
In order to use the resource-managed allocation of a struct drm_device
instance, it is recommended to embed the drm_device instance within the
driver-specific structure.
As there is already a helper to convert a drm_device to a nouveau_drm
struct, this adds an inverse function and changes all direct references
to the pointer within nouveau_drm to use the function. It also adds a
helper to convert directly to a generic device structure.
This allows us to switch from maintaining a pointer in nouveau_drm to
embedding the structure by simply altering the helper function
implementations.
This patch should introduce no functional changes.
Signed-off-by: Jeremy Cline
---
drivers/gpu/drm/nouveau/dispnv04/crtc.c | 10 +++---
drivers/gpu/drm/nouveau/dispnv50/base.c | 2 +-
drivers/gpu/drm/nouveau/dispnv50/base507c.c | 7 ++--
drivers/gpu/drm/nouveau/dispnv50/core.c | 2 +-
drivers/gpu/drm/nouveau/dispnv50/core507d.c | 2 +-
drivers/gpu/drm/nouveau/dispnv50/curs.c | 2 +-
drivers/gpu/drm/nouveau/dispnv50/curs507a.c | 5 +--
drivers/gpu/drm/nouveau/dispnv50/disp.c | 17 +-
drivers/gpu/drm/nouveau/dispnv50/oimm.c | 2 +-
drivers/gpu/drm/nouveau/dispnv50/oimm507b.c | 2 +-
drivers/gpu/drm/nouveau/dispnv50/ovly.c | 2 +-
drivers/gpu/drm/nouveau/dispnv50/ovly507e.c | 5 +--
drivers/gpu/drm/nouveau/dispnv50/wimm.c | 2 +-
drivers/gpu/drm/nouveau/dispnv50/wimmc37b.c | 2 +-
drivers/gpu/drm/nouveau/dispnv50/wndw.c | 2 +-
drivers/gpu/drm/nouveau/dispnv50/wndwc37e.c | 5 +--
drivers/gpu/drm/nouveau/nouveau_bo.c| 16 -
drivers/gpu/drm/nouveau/nouveau_debugfs.c | 9 +++---
drivers/gpu/drm/nouveau/nouveau_display.c | 16 -
drivers/gpu/drm/nouveau/nouveau_dmem.c | 17 +-
drivers/gpu/drm/nouveau/nouveau_drm.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_drv.h | 36 -
drivers/gpu/drm/nouveau/nouveau_fbcon.c | 19 +--
drivers/gpu/drm/nouveau/nouveau_gem.c | 8 ++---
drivers/gpu/drm/nouveau/nouveau_svm.c | 4 +--
drivers/gpu/drm/nouveau/nouveau_ttm.c | 4 +--
drivers/gpu/drm/nouveau/nouveau_vga.c | 8 ++---
27 files changed, 124 insertions(+), 84 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c
b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
index f9e962fd94d0..3e1b00f3086e 100644
--- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c
+++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
@@ -915,7 +915,7 @@ nv04_crtc_mode_set_base_atomic(struct drm_crtc *crtc,
int x, int y, enum mode_set_atomic state)
{
struct nouveau_drm *drm = nouveau_drm(crtc->dev);
- struct drm_device *dev = drm->dev;
+ struct drm_device *dev = nouveau_to_drm_dev(drm);
if (state == ENTER_ATOMIC_MODE_SET)
nouveau_fbcon_accel_save_disable(dev);
@@ -990,7 +990,7 @@ nv04_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file
*file_priv,
uint32_t buffer_handle, uint32_t width, uint32_t height)
{
struct nouveau_drm *drm = nouveau_drm(crtc->dev);
- struct drm_device *dev = drm->dev;
+ struct drm_device *dev = nouveau_to_drm_dev(drm);
struct nouveau_crtc *nv_crtc = nouveau_crtc(crtc);
struct nouveau_bo *cursor = NULL;
struct drm_gem_object *gem;
@@ -1050,7 +1050,7 @@ nv04_finish_page_flip(struct nouveau_channel *chan,
{
struct nouveau_fence_chan *fctx = chan->fence;
struct nouveau_drm *drm = chan->drm;
- struct drm_device *dev = drm->dev;
+ struct drm_device *dev = nouveau_to_drm_dev(drm);
struct nv04_page_flip_state *s;
unsigned long flags;
@@ -1088,7 +1088,7 @@ nv04_flip_complete(struct nvif_notify *notify)
struct nv04_page_flip_state state;
if (!nv04_finish_page_flip(chan, )) {
- nv_set_crtc_base(drm->dev, drm_crtc_index(state.crtc),
+ nv_set_crtc_base(nouveau_to_drm_dev(drm),
drm_crtc_index(state.crtc),
state.offset + state.crtc->y *
state.pitch + state.crtc->x *
state.bpp / 8);
@@ -1106,7 +1106,7 @@ nv04_page_flip_emit(struct nouveau_channel *chan,
{
struct nouveau_fence_chan *fctx = chan->fence;
struct nouveau_drm *drm = chan->drm;
- struct drm_device *dev = drm->dev;
+ struct drm_device *dev = nouveau_to_drm_dev(drm);
struct nvif_push *push = chan->chan.push;
unsigned long flags;
int ret;
diff --git a/drivers/gpu/drm/nouveau/dispnv50/base.c
b/drivers/gpu/drm/nouveau/dispnv50/base.c
index 7c752acf2b48..cd70456dd37f 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/base.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/base.c
@@ -40,7 +40,7 @@ nv50_base_new(struct nouveau_drm *drm, int head, struct
nv50_wndw **pwndw)
{ NV50_DISP_BASE_CHANNEL_DMA, 0, base507c_new },
[PATCH 3/3] drm/nouveau: begin documenting core nouveau structures
Start on documentation for the Nouveau device structure and the NVIF
client structure it uses. This documentation is not complete as the
structures are non-trivial and I am not familiar with large portions of
them.
Signed-off-by: Jeremy Cline
---
drivers/gpu/drm/nouveau/nouveau_drv.h | 67 +++
1 file changed, 67 insertions(+)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
b/drivers/gpu/drm/nouveau/nouveau_drv.h
index cf6c33e52a5c..cf83d4bf3c6c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -90,8 +90,20 @@ enum nouveau_drm_handle {
NVDRM_NVSW= 0x,
};
+/**
+ * struct nouveau_cli - A DRM-specific NVIF client.
+ *
+ * This encapsulates a NVIF client and is intended to be the sole interface
+ * between the DRM APIs and NVKM. An instance of this structure is allocated
+ * for each userspace client when they open the device file. Additionally,
+ * there are several allocated strictly for the kernel's use.
+ */
struct nouveau_cli {
struct nvif_client base;
+
+ /**
+* @drm: A reference to the device that the client is associated with.
+*/
struct nouveau_drm *drm;
struct mutex mutex;
@@ -101,6 +113,9 @@ struct nouveau_cli {
struct nouveau_vmm svm;
const struct nvif_mclass *mem;
+ /**
+* @head: The list entry for this client in the @drm device's list of
clients.
+*/
struct list_head head;
void *abi16;
struct list_head objects;
@@ -108,13 +123,36 @@ struct nouveau_cli {
char name[32];
struct work_struct work;
+
+ /**
+* @worker: List of pending nouveau_cli_work associated with
this client.
+*/
struct list_head worker;
+
+ /**
+* @lock: Protects the @worker list. Additionally, this lock on the
+* @drm.master instance is used to serialize destruction of the @base
+* member in this structure, as well as the destruction of the
+* nvif_mem embedded in nouveau_mem instances.
+*/
struct mutex lock;
};
+/**
+ * struct nouveau_cli_work - A pending work item for an NVIF client.
+ */
struct nouveau_cli_work {
void (*func)(struct nouveau_cli_work *);
+
+ /**
+* @cli: Reference to the NVIF client this work belongs to.
+*/
struct nouveau_cli *cli;
+
+ /**
+* @head: The list entry for this work item in the nouveau_cli
+* worker list.
+*/
struct list_head head;
struct dma_fence *fence;
@@ -133,9 +171,32 @@ nouveau_cli(struct drm_file *fpriv)
#include
#include
+/**
+ * struct nouveau_drm - The nouveau-specific device structure.
+ *
+ * This structure is allocated for a device when it is probed and keeps track
+ * of all the nouveau-specific device details. The lifetime of this structure
+ * is the same as the lifetime of a drm_device and is managed by the
+ * DRM layer.
+ */
struct nouveau_drm {
+ /**
+* @parent: Implementation of the interface required to use the
NVIF_DEBUG
+* and NVIF_ERROR macros
+*/
struct nvif_parent parent;
+
+ /**
+* @master: This NVIF client is used to initialize the NVIF driver and
used
+* for TTM memory allocations. It is the root of the NVIF object tree.
+*/
struct nouveau_cli master;
+
+ /**
+* @client: This NVIF client is used by the DRM layer to interact with
+* the NVKM layer for everything except TTM memory allocations. It, and
+* all other clients, are children of the primary (@master) client.
+*/
struct nouveau_cli client;
/**
@@ -143,6 +204,12 @@ struct nouveau_drm {
*/
struct drm_device drm_dev;
+ /**
+* @clients: List of all nouveau_cli allocated for userspace
+* associated with this DRM device. Clients are allocated when the DRM
+* file is opened and deallocated when the file is closed. This list is
+* protected by the mutex in @client.
+*/
struct list_head clients;
u8 old_pm_cap;
--
2.28.0
[PATCH 0/3] drm/nouveau: extend the lifetime of nouveau_drm
Hi folks, Currently, when the device is removed (or the driver is unbound) the nouveau_drm structure de-allocated. However, it's still accessible from and used by some DRM layer callbacks. For example, file handles can be closed after the device has been removed (physically or otherwise). This series converts the Nouveau device structure to be allocated and de-allocated with the devm_drm_dev_alloc() API. In the future, additional resources that should be bound to the lifetime of the drm_device can be added, and the drmm_add_action() APIs offer a nice hook for arbitrary cleanup actions before the drm_device is destroyed, so I suspect much of the current cleanup code in Nouveau would benefit from some refactoring to use this. Finally, although not *strictly* necessary for this series, I included some documentation for structures I investigated for this work. Jeremy Cline (3): drm/nouveau: Use helper to convert nouveau_drm to drm_device drm/nouveau: manage nouveau_drm lifetime with devres drm/nouveau: begin documenting core nouveau structures drivers/gpu/drm/nouveau/dispnv04/crtc.c | 10 +- drivers/gpu/drm/nouveau/dispnv50/base.c | 2 +- drivers/gpu/drm/nouveau/dispnv50/base507c.c | 7 +- drivers/gpu/drm/nouveau/dispnv50/core.c | 2 +- drivers/gpu/drm/nouveau/dispnv50/core507d.c | 2 +- drivers/gpu/drm/nouveau/dispnv50/curs.c | 2 +- drivers/gpu/drm/nouveau/dispnv50/curs507a.c | 5 +- drivers/gpu/drm/nouveau/dispnv50/disp.c | 17 +-- drivers/gpu/drm/nouveau/dispnv50/oimm.c | 2 +- drivers/gpu/drm/nouveau/dispnv50/oimm507b.c | 2 +- drivers/gpu/drm/nouveau/dispnv50/ovly.c | 2 +- drivers/gpu/drm/nouveau/dispnv50/ovly507e.c | 5 +- drivers/gpu/drm/nouveau/dispnv50/wimm.c | 2 +- drivers/gpu/drm/nouveau/dispnv50/wimmc37b.c | 2 +- drivers/gpu/drm/nouveau/dispnv50/wndw.c | 2 +- drivers/gpu/drm/nouveau/dispnv50/wndwc37e.c | 5 +- drivers/gpu/drm/nouveau/nouveau_bo.c| 16 ++- drivers/gpu/drm/nouveau/nouveau_debugfs.c | 9 +- drivers/gpu/drm/nouveau/nouveau_display.c | 16 +-- drivers/gpu/drm/nouveau/nouveau_dmem.c | 17 +-- drivers/gpu/drm/nouveau/nouveau_drm.c | 41 drivers/gpu/drm/nouveau/nouveau_drv.h | 111 +++- drivers/gpu/drm/nouveau/nouveau_fbcon.c | 19 ++-- drivers/gpu/drm/nouveau/nouveau_gem.c | 8 +- drivers/gpu/drm/nouveau/nouveau_svm.c | 4 +- drivers/gpu/drm/nouveau/nouveau_ttm.c | 4 +- drivers/gpu/drm/nouveau/nouveau_vga.c | 8 +- 27 files changed, 216 insertions(+), 106 deletions(-) -- 2.28.0
[PATCH 2/3] drm/nouveau: Add a dedicated mutex for the clients list
Rather than protecting the nouveau_drm clients list with the lock within
the "client" nouveau_cli, add a dedicated lock to serialize access to
the list. This is both clearer and necessary to avoid lockdep being
upset with us when we need to iterate through all the clients in the
list and potentially lock their mutex, which is the same class as the
lock protecting the entire list.
Signed-off-by: Jeremy Cline
---
drivers/gpu/drm/nouveau/nouveau_drm.c | 9 +
drivers/gpu/drm/nouveau/nouveau_drv.h | 5 +
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 4fe4d664c5f2..d182b877258a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -557,6 +557,7 @@ nouveau_drm_device_init(struct drm_device *dev)
nvkm_dbgopt(nouveau_debug, "DRM");
INIT_LIST_HEAD(>clients);
+ mutex_init(>clients_lock);
spin_lock_init(>tile.lock);
/* workaround an odd issue on nvc1 by disabling the device's
@@ -1089,9 +1090,9 @@ nouveau_drm_open(struct drm_device *dev, struct drm_file
*fpriv)
fpriv->driver_priv = cli;
- mutex_lock(>client.mutex);
+ mutex_lock(>clients_lock);
list_add(>head, >clients);
- mutex_unlock(>client.mutex);
+ mutex_unlock(>clients_lock);
done:
if (ret && cli) {
@@ -1117,9 +1118,9 @@ nouveau_drm_postclose(struct drm_device *dev, struct
drm_file *fpriv)
nouveau_abi16_fini(cli->abi16);
mutex_unlock(>mutex);
- mutex_lock(>client.mutex);
+ mutex_lock(>clients_lock);
list_del(>head);
- mutex_unlock(>client.mutex);
+ mutex_unlock(>clients_lock);
nouveau_cli_fini(cli);
kfree(cli);
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
b/drivers/gpu/drm/nouveau/nouveau_drv.h
index 9d04d1b36434..550e5f335146 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -141,6 +141,11 @@ struct nouveau_drm {
struct list_head clients;
+ /**
+* @clients_lock: Protects access to the @clients list of
nouveau_cli.
+*/
+ struct mutex clients_lock;
+
u8 old_pm_cap;
struct {
--
2.28.0
[PATCH 1/3] drm/nouveau: use drm_dev_unplug() during device removal
Nouveau does not currently support hot-unplugging, but it still makes sense to switch from drm_dev_unregister() to drm_dev_unplug(). drm_dev_unplug() calls drm_dev_unregister() after marking the device as unplugged, but only after any device critical sections are finished. Since nouveau isn't using drm_dev_enter() and drm_dev_exit(), there are no critical sections so this is nearly functionally equivalent. However, the DRM layer does check to see if the device is unplugged, and if it is returns appropriate error codes. In the future nouveau can add critical sections in order to truly support hot-unplugging. Signed-off-by: Jeremy Cline --- drivers/gpu/drm/nouveau/nouveau_drm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index d141a5f004af..4fe4d664c5f2 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -792,7 +792,7 @@ nouveau_drm_device_remove(struct drm_device *dev) struct nvkm_client *client; struct nvkm_device *device; - drm_dev_unregister(dev); + drm_dev_unplug(dev); dev->irq_enabled = false; client = nvxx_client(>client.base); -- 2.28.0
[PATCH 3/3] drm/nouveau: clean up all clients on device removal
The postclose handler can run after the device has been removed (or the
driver has been unbound) since userspace clients are free to hold the
file open the file as long as they want. Because the device removal
callback frees the entire nouveau_drm structure, any reference to it in
the postclose handler will result in a use-after-free.
To reproduce this, one must simply open the device file, unbind the
driver (or physically remove the device), and then close the device
file. This was found and can be reproduced easily with the IGT
core_hotunplug tests.
To avoid this, all clients are cleaned up in the device finialization
rather than deferring it to the postclose handler, and the postclose
handler is protected by a critical section which ensures the
drm_dev_unplug() and the postclose handler won't race.
This is not an ideal fix, since as I understand the proposed plan for
the kernel<->userspace interface for hotplug support, destroying the
client before the file is closed will cause problems. However, I believe
to properly fix this issue, the lifetime of the nouveau_drm structure
needs to be extended to match the drm_device, and this proved to be a
rather invasive change. Thus, I've broken this out so the fix can be
easily backported.
Signed-off-by: Jeremy Cline
---
drivers/gpu/drm/nouveau/nouveau_drm.c | 30 +++
1 file changed, 30 insertions(+)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index d182b877258a..74fab777f4d0 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -628,6 +628,7 @@ nouveau_drm_device_init(struct drm_device *dev)
static void
nouveau_drm_device_fini(struct drm_device *dev)
{
+ struct nouveau_cli *cli, *temp_cli;
struct nouveau_drm *drm = nouveau_drm(dev);
if (nouveau_pmops_runtime()) {
@@ -652,6 +653,24 @@ nouveau_drm_device_fini(struct drm_device *dev)
nouveau_ttm_fini(drm);
nouveau_vga_fini(drm);
+ /*
+* There may be existing clients from as-yet unclosed files. For now,
+* clean them up here rather than deferring until the file is closed,
+* but this likely not correct if we want to support hot-unplugging
+* properly.
+*/
+ mutex_lock(>clients_lock);
+ list_for_each_entry_safe(cli, temp_cli, >clients, head) {
+ list_del(>head);
+ mutex_lock(>mutex);
+ if (cli->abi16)
+ nouveau_abi16_fini(cli->abi16);
+ mutex_unlock(>mutex);
+ nouveau_cli_fini(cli);
+ kfree(cli);
+ }
+ mutex_unlock(>clients_lock);
+
nouveau_cli_fini(>client);
nouveau_cli_fini(>master);
nvif_parent_dtor(>parent);
@@ -1110,6 +1129,16 @@ nouveau_drm_postclose(struct drm_device *dev, struct
drm_file *fpriv)
{
struct nouveau_cli *cli = nouveau_cli(fpriv);
struct nouveau_drm *drm = nouveau_drm(dev);
+ int dev_index;
+
+ /*
+* The device is gone, and as it currently stands all clients are
+* cleaned up in the removal codepath. In the future this may change
+* so that we can support hot-unplugging, but for now we immediately
+* return to avoid a double-free situation.
+*/
+ if (!drm_dev_enter(dev, _index))
+ return;
pm_runtime_get_sync(dev->dev);
@@ -1126,6 +1155,7 @@ nouveau_drm_postclose(struct drm_device *dev, struct
drm_file *fpriv)
kfree(cli);
pm_runtime_mark_last_busy(dev->dev);
pm_runtime_put_autosuspend(dev->dev);
+ drm_dev_exit(dev_index);
}
static const struct drm_ioctl_desc
--
2.28.0
[PATCH 0/3] drm/nouveau: fix a use-after-free in postclose()
This series fixes a number of use-after-frees in nouveau's postclose() handler. It was discovered by pointing IGT's core_hotunplug tests at a nouveau device, but the steps to reproduce it are simple: 1. Open the device file 2. Unbind the driver or remove the device 3. Close the file opened in step 1. During the device removal, the nouveau_drm structure is de-allocated, but is dereferenced in the postclose() handler. One obvious solution is to ensure all the operations in the postclose() handler are valid by extending the lifetime of the nouveau_drm structure. This is possible with the new devm_drm_dev_alloc() interface, but the change is somewhat invasive so I thought it best to submit that work separately. Instead, we make use of the drm_dev_unplug() API, clean up all clients in the device removal call, and check to make sure the device has not been unplugged in the postclose() handler. While this does not enable hot-unplug support for nouveau, it's enough to avoid crashing the kernel and leads to all the core_hotunplug tests to pass. Jeremy Cline (3): drm/nouveau: use drm_dev_unplug() during device removal drm/nouveau: Add a dedicated mutex for the clients list drm/nouveau: clean up all clients on device removal drivers/gpu/drm/nouveau/nouveau_drm.c | 39 +++ drivers/gpu/drm/nouveau/nouveau_drv.h | 5 2 files changed, 39 insertions(+), 5 deletions(-) -- 2.28.0
Re: [PATCH v2 1/2] drm/nouveau: return temperatures in temp_get() via parameter
On Wed, Sep 16, 2020 at 10:03:22PM +0200, Karol Herbst wrote:
> On Wed, Sep 16, 2020 at 10:01 PM Karol Herbst wrote:
> >
> > On Wed, Sep 16, 2020 at 9:47 PM Jeremy Cline wrote:
> > >
> > > The temp_get() function currently returns negative error numbers or a
> > > temperature. However, the thermal sensors can (in theory) measure
> > > negative temperatures. Some implementations of temp_get() correctly
> > > clamp negative temperature readings to 0 so that users do not mistake
> > > them for errors, but some, like gp100_temp_get(), do not.
> > >
> > > Rather than relying on implementations remembering to clamp values,
> > > dedicate the function return value to error codes and accept a pointer
> > > to an integer where the temperature reading should be stored.
> > >
> > > Signed-off-by: Jeremy Cline
> > > ---
> > > .../drm/nouveau/include/nvkm/subdev/therm.h | 2 +-
> > > drivers/gpu/drm/nouveau/nouveau_hwmon.c | 12 ++--
> > > .../gpu/drm/nouveau/nvkm/subdev/therm/base.c | 19 ++-
> > > .../gpu/drm/nouveau/nvkm/subdev/therm/g84.c | 11 ++-
> > > .../gpu/drm/nouveau/nvkm/subdev/therm/gp100.c | 11 ++-
> > > .../gpu/drm/nouveau/nvkm/subdev/therm/nv40.c | 9 +++--
> > > .../gpu/drm/nouveau/nvkm/subdev/therm/nv50.c | 9 +++--
> > > .../gpu/drm/nouveau/nvkm/subdev/therm/priv.h | 17 +++--
> > > .../gpu/drm/nouveau/nvkm/subdev/therm/temp.c | 12
> > > 9 files changed, 62 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
> > > b/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
> > > index 62c34f98c930..bfe9779216fc 100644
> > > --- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
> > > +++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
> > > @@ -99,7 +99,7 @@ struct nvkm_therm {
> > > bool clkgating_enabled;
> > > };
> > >
> > > -int nvkm_therm_temp_get(struct nvkm_therm *);
> > > +int nvkm_therm_temp_get(struct nvkm_therm *therm, int *temp);
> > > int nvkm_therm_fan_sense(struct nvkm_therm *);
> > > int nvkm_therm_cstate(struct nvkm_therm *, int, int);
> > > void nvkm_therm_clkgate_init(struct nvkm_therm *,
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> > > b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> > > index 1c3104d20571..aff6aa296678 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> > > @@ -325,7 +325,7 @@ nouveau_temp_is_visible(const void *data, u32 attr,
> > > int channel)
> > > struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
> > > struct nvkm_therm *therm = nvxx_therm(>client.device);
> > >
> > > - if (!therm || !therm->attr_get || nvkm_therm_temp_get(therm) < 0)
> > > + if (!therm || !therm->attr_get || nvkm_therm_temp_get(therm,
> > > NULL) < 0)
> > > return 0;
> > >
> > > switch (attr) {
> > > @@ -419,7 +419,7 @@ nouveau_temp_read(struct device *dev, u32 attr, int
> > > channel, long *val)
> > > struct drm_device *drm_dev = dev_get_drvdata(dev);
> > > struct nouveau_drm *drm = nouveau_drm(drm_dev);
> > > struct nvkm_therm *therm = nvxx_therm(>client.device);
> > > - int ret;
> > > + int ret = 0, temp;
> > >
> > > if (!therm || !therm->attr_get)
> > > return -EOPNOTSUPP;
> > > @@ -428,8 +428,8 @@ nouveau_temp_read(struct device *dev, u32 attr, int
> > > channel, long *val)
> > > case hwmon_temp_input:
> > > if (drm_dev->switch_power_state != DRM_SWITCH_POWER_ON)
> > > return -EINVAL;
> > > - ret = nvkm_therm_temp_get(therm);
> > > - *val = ret < 0 ? ret : (ret * 1000);
> > > + ret = nvkm_therm_temp_get(therm, );
> > > + *val = temp * 1000;
> > > break;
> > > case hwmon_temp_max:
> > > *val = therm->attr_get(therm,
> > > NVKM_THERM_ATTR_THRS_DOWN_CLK)
> > > @@ -459,7 +459,7 @@ nouveau_temp_read(struct device *dev, u32 attr, int
> > > channel, long *val)
> > > return -EOPNOTSUPP;
> > > }
>
[PATCH v2 1/2] drm/nouveau: return temperatures in temp_get() via parameter
The temp_get() function currently returns negative error numbers or a
temperature. However, the thermal sensors can (in theory) measure
negative temperatures. Some implementations of temp_get() correctly
clamp negative temperature readings to 0 so that users do not mistake
them for errors, but some, like gp100_temp_get(), do not.
Rather than relying on implementations remembering to clamp values,
dedicate the function return value to error codes and accept a pointer
to an integer where the temperature reading should be stored.
Signed-off-by: Jeremy Cline
---
.../drm/nouveau/include/nvkm/subdev/therm.h | 2 +-
drivers/gpu/drm/nouveau/nouveau_hwmon.c | 12 ++--
.../gpu/drm/nouveau/nvkm/subdev/therm/base.c | 19 ++-
.../gpu/drm/nouveau/nvkm/subdev/therm/g84.c | 11 ++-
.../gpu/drm/nouveau/nvkm/subdev/therm/gp100.c | 11 ++-
.../gpu/drm/nouveau/nvkm/subdev/therm/nv40.c | 9 +++--
.../gpu/drm/nouveau/nvkm/subdev/therm/nv50.c | 9 +++--
.../gpu/drm/nouveau/nvkm/subdev/therm/priv.h | 17 +++--
.../gpu/drm/nouveau/nvkm/subdev/therm/temp.c | 12
9 files changed, 62 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
b/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
index 62c34f98c930..bfe9779216fc 100644
--- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
+++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
@@ -99,7 +99,7 @@ struct nvkm_therm {
bool clkgating_enabled;
};
-int nvkm_therm_temp_get(struct nvkm_therm *);
+int nvkm_therm_temp_get(struct nvkm_therm *therm, int *temp);
int nvkm_therm_fan_sense(struct nvkm_therm *);
int nvkm_therm_cstate(struct nvkm_therm *, int, int);
void nvkm_therm_clkgate_init(struct nvkm_therm *,
diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
index 1c3104d20571..aff6aa296678 100644
--- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
@@ -325,7 +325,7 @@ nouveau_temp_is_visible(const void *data, u32 attr, int
channel)
struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
struct nvkm_therm *therm = nvxx_therm(>client.device);
- if (!therm || !therm->attr_get || nvkm_therm_temp_get(therm) < 0)
+ if (!therm || !therm->attr_get || nvkm_therm_temp_get(therm, NULL) < 0)
return 0;
switch (attr) {
@@ -419,7 +419,7 @@ nouveau_temp_read(struct device *dev, u32 attr, int
channel, long *val)
struct drm_device *drm_dev = dev_get_drvdata(dev);
struct nouveau_drm *drm = nouveau_drm(drm_dev);
struct nvkm_therm *therm = nvxx_therm(>client.device);
- int ret;
+ int ret = 0, temp;
if (!therm || !therm->attr_get)
return -EOPNOTSUPP;
@@ -428,8 +428,8 @@ nouveau_temp_read(struct device *dev, u32 attr, int
channel, long *val)
case hwmon_temp_input:
if (drm_dev->switch_power_state != DRM_SWITCH_POWER_ON)
return -EINVAL;
- ret = nvkm_therm_temp_get(therm);
- *val = ret < 0 ? ret : (ret * 1000);
+ ret = nvkm_therm_temp_get(therm, );
+ *val = temp * 1000;
break;
case hwmon_temp_max:
*val = therm->attr_get(therm, NVKM_THERM_ATTR_THRS_DOWN_CLK)
@@ -459,7 +459,7 @@ nouveau_temp_read(struct device *dev, u32 attr, int
channel, long *val)
return -EOPNOTSUPP;
}
- return 0;
+ return ret;
}
static int
@@ -735,7 +735,7 @@ nouveau_hwmon_init(struct drm_device *dev)
hwmon->dev = dev;
if (therm && therm->attr_get && therm->attr_set) {
- if (nvkm_therm_temp_get(therm) >= 0)
+ if (nvkm_therm_temp_get(therm, NULL) >= 0)
special_groups[i++] = _auto_point_sensor_group;
if (therm->fan_get && therm->fan_get(therm) >= 0)
special_groups[i++] = _fan_sensor_group;
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/base.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/base.c
index 4a4d1e224126..e7ffea1512e6 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/base.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/base.c
@@ -27,10 +27,15 @@
#include
int
-nvkm_therm_temp_get(struct nvkm_therm *therm)
+nvkm_therm_temp_get(struct nvkm_therm *therm, int *temp)
{
+ int ignored_reading;
+
+ if (temp == NULL)
+ temp = _reading;
+
if (therm->func->temp_get)
- return therm->func->temp_get(therm);
+ return therm->func->temp_get(therm, temp);
return -ENODEV;
}
@@ -40,9 +45,11 @@ nvkm_therm_update_trip(struct nvkm_therm *therm)
struct nvbios_therm_t
[PATCH v2 2/2] drm/nouveau: Add fine-grain temperature reporting
Commit d32656373857 ("drm/nouveau/therm/gp100: initial implementation of
new gp1xx temperature sensor") added support for reading finer-grain
temperatures, but continued to report temperatures in 1 degree Celsius
increments via nvkm_therm_temp_get().
This alters the temp_get() API to return millidegrees rather than
degrees, adjusts all implementations, and changes users that need integer
Celsius to use the new nvkm_therm_temp_get_c() helper function. Since
there are now two units of measurement floating around, structs that
store temperature now include documentation to make it clear what the
unit is.
Signed-off-by: Jeremy Cline
---
.../nouveau/include/nvkm/subdev/bios/therm.h | 13 ++
.../drm/nouveau/include/nvkm/subdev/therm.h | 26 +++
drivers/gpu/drm/nouveau/nouveau_hwmon.c | 2 +-
.../gpu/drm/nouveau/nvkm/subdev/therm/base.c | 14 --
.../gpu/drm/nouveau/nvkm/subdev/therm/g84.c | 4 +--
.../gpu/drm/nouveau/nvkm/subdev/therm/gp100.c | 2 +-
.../gpu/drm/nouveau/nvkm/subdev/therm/nv40.c | 2 +-
.../gpu/drm/nouveau/nvkm/subdev/therm/nv50.c | 2 +-
.../gpu/drm/nouveau/nvkm/subdev/therm/temp.c | 4 +--
9 files changed, 59 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/bios/therm.h
b/drivers/gpu/drm/nouveau/include/nvkm/subdev/bios/therm.h
index 0fb8a3480871..fdf23214cc69 100644
--- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/bios/therm.h
+++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/bios/therm.h
@@ -1,6 +1,12 @@
/* SPDX-License-Identifier: MIT */
#ifndef __NVBIOS_THERM_H__
#define __NVBIOS_THERM_H__
+
+/**
+ * struct nvbios_therm_threshold - The threshold of a thermal event.
+ * @temp: The temperature, in degrees Celsius, of the thermal threshold.
+ * @hysteresis: The hysteresis of this threshold, in degrees Celsius.
+ */
struct nvbios_therm_threshold {
u8 temp;
u8 hysteresis;
@@ -29,6 +35,13 @@ enum nvbios_therm_fan_type {
/* no vbios have more than 6 */
#define NVKM_TEMP_FAN_TRIP_MAX 10
+
+/**
+ * struct nvbios_therm_trip_point - Represents a thermal trip point.
+ * @fan_duty: The fan's duty cycle as a percentage. Valid values are 0-100.
+ * @temp: The temperature of this trip point, in degrees Celsius.
+ * @hysteresis: the hysteresis to use at this trip point, in degrees Celsius.
+ */
struct nvbios_therm_trip_point {
int fan_duty;
int temp;
diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
b/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
index bfe9779216fc..e817bb9c9505 100644
--- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
+++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
@@ -99,7 +99,33 @@ struct nvkm_therm {
bool clkgating_enabled;
};
+/**
+ * nvkm_therm_temp_get() - get the temperature in millidegrees Celsius
+ * @therm: The thermal device to read from.
+ * @temp: A pointer to write the temperature reading to. Passing NULL is
+ * allowed, useful if you only need to check that it's possible to
+ * read a temperature.
+ *
+ * Note that because some cards are only capable of reporting temperatures in
+ * integer degrees Celsius or, if they support fractional degrees, not support
+ * millidegree accuracy, the accuracy of this interface is hardware dependent.
+ *
+ * Returns: Zero on success, or a negative error code on failure.
+ */
int nvkm_therm_temp_get(struct nvkm_therm *therm, int *temp);
+
+/**
+ * nvkm_therm_temp_get_c() - get the temperature in degrees Celsius
+ * @therm: The thermal device to read from.
+ * @temp: A pointer to write the temperature reading to.
+ *
+ * A convenient wrapper for nvkm_therm_temp_get() that converts to degrees
+ * Celsius.
+ *
+ * Returns: Zero on success, or a negative error code on failure.
+ */
+int nvkm_therm_temp_get_c(struct nvkm_therm *therm, int *temp);
+
int nvkm_therm_fan_sense(struct nvkm_therm *);
int nvkm_therm_cstate(struct nvkm_therm *, int, int);
void nvkm_therm_clkgate_init(struct nvkm_therm *,
diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
index aff6aa296678..1363068f44c1 100644
--- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
@@ -429,7 +429,7 @@ nouveau_temp_read(struct device *dev, u32 attr, int
channel, long *val)
if (drm_dev->switch_power_state != DRM_SWITCH_POWER_ON)
return -EINVAL;
ret = nvkm_therm_temp_get(therm, );
- *val = temp * 1000;
+ *val = temp;
break;
case hwmon_temp_max:
*val = therm->attr_get(therm, NVKM_THERM_ATTR_THRS_DOWN_CLK)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/base.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/base.c
index e7ffea1512e6..a7bb1e6b6169 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/base.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/base.c
[PATCH v2 0/2] Add fine-grain temperature reporting
Hi folks, This series adjusts the temp_get() interface in the thermal functions to report milli-degrees, and additionally alters the way errors are reported. As I went through all the users and implementations I realized that Pascal's temp_get() implementation didn't include turning negative temperatures to 0 like other implementations, so I separated error reporting from the temperature in the API. Couple of things I'm on the fence about here: * I allowed the nvkm_therm_temp_get() functions to accept NULL as a way to check if temperature reading is available rather than adding a separate helper, but was torn about whether this is clearer than a separate helper function. * I added a WARN_ON in places that previously called nvkm_therm_temp_get() and didn't check the return value for an error. This may not be a reasonable error handling method. Jeremy Cline (2): drm/nouveau: return temperatures in temp_get() via parameter drm/nouveau: Add fine-grain temperature reporting .../nouveau/include/nvkm/subdev/bios/therm.h | 13 + .../drm/nouveau/include/nvkm/subdev/therm.h | 28 ++- drivers/gpu/drm/nouveau/nouveau_hwmon.c | 12 .../gpu/drm/nouveau/nvkm/subdev/therm/base.c | 28 +++ .../gpu/drm/nouveau/nvkm/subdev/therm/g84.c | 11 .../gpu/drm/nouveau/nvkm/subdev/therm/gp100.c | 11 .../gpu/drm/nouveau/nvkm/subdev/therm/nv40.c | 9 ++ .../gpu/drm/nouveau/nvkm/subdev/therm/nv50.c | 9 ++ .../gpu/drm/nouveau/nvkm/subdev/therm/priv.h | 17 +-- .../gpu/drm/nouveau/nvkm/subdev/therm/temp.c | 12 +--- 10 files changed, 110 insertions(+), 40 deletions(-) -- 2.26.2
Re: [Nouveau] [PATCH] drm/nouveau: Add fine-grain temperature reporting
On Wed, Sep 09, 2020 at 10:22:14AM +0200, Karol Herbst wrote:
> On Wed, Sep 9, 2020 at 6:06 AM Ben Skeggs wrote:
> >
> > On Thu, 13 Aug 2020 at 06:50, Jeremy Cline wrote:
> > >
> > > Commit d32656373857 ("drm/nouveau/therm/gp100: initial implementation of
> > > new gp1xx temperature sensor") added support for reading finer-grain
> > > temperatures, but continued to report temperatures in 1 degree Celsius
> > > increments via nvkm_therm_temp_get().
> > >
> > > Rather than altering nvkm_therm_temp_get() to report finer-grain
> > > temperatures, which would be inconvenient for other users of the
> > > function, a second interface has been added to line up with hwmon's
> > > native unit of temperature.
> > Hey Jeremy,
> >
> > Sorry this slipped past me until now. I'm OK with adding support for
> > millidegree temperature reporting, but don't think we need to keep
> > both interfaces around and would rather see the existing code
> > converted to return millidegrees (even on GPUs that don't support it)
> > instead of degrees.
> >
> > Thanks!
> > Ben.
> >
>
> just a note as I was trying something like that in the past: we have a
> lot of code which fetches the temperature and there are a lot of
> places where we would then do a divide by 1000, so having a wrapper
> function at least makes sense. If we want to keep the func pointers?
> well.. I guess the increased CPU overhead wouldn't be as bad if all
> sub classes do this static * 1000 as well either. I just think we
> should have both interfaces in subdev/therm.h so we can just keep most
> of the current code as is.
>
Indeed. My initial plan was to change the meaning of temp_get() and
adjust all the users, but there's around a dozen of them and based on my
understanding of the users none of them cared much about such accuracy.
However, I do find having one way to do things appealing, so if there's
a strong preference for it, I'm happy to produce a somewhat more
invasive patch where all users expect millidegrees.
- Jeremy
[PATCH] drm/nouveau: Add fine-grain temperature reporting
Commit d32656373857 ("drm/nouveau/therm/gp100: initial implementation of
new gp1xx temperature sensor") added support for reading finer-grain
temperatures, but continued to report temperatures in 1 degree Celsius
increments via nvkm_therm_temp_get().
Rather than altering nvkm_therm_temp_get() to report finer-grain
temperatures, which would be inconvenient for other users of the
function, a second interface has been added to line up with hwmon's
native unit of temperature.
Signed-off-by: Jeremy Cline
---
.../drm/nouveau/include/nvkm/subdev/therm.h | 18 +
drivers/gpu/drm/nouveau/nouveau_hwmon.c | 4 +--
.../gpu/drm/nouveau/nvkm/subdev/therm/base.c | 16
.../gpu/drm/nouveau/nvkm/subdev/therm/gp100.c | 25 +--
.../gpu/drm/nouveau/nvkm/subdev/therm/priv.h | 1 +
5 files changed, 60 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
b/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
index 62c34f98c930..7b9928dd001c 100644
--- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
+++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
@@ -100,6 +100,24 @@ struct nvkm_therm {
};
int nvkm_therm_temp_get(struct nvkm_therm *);
+
+/**
+ * nvkm_therm_temp_millidegree_get() - get the temperature in millidegrees
+ * @therm: The thermal device to read from.
+ *
+ * This interface reports temperatures in units of millidegree Celsius to
+ * align with the hwmon API. Some cards may only be capable of reporting in
+ * units of Celsius, and those that report finer grain temperatures may not be
+ * capable of millidegree Celsius accuracy,
+ *
+ * For cases where millidegree temperature is too fine-grain, the
+ * nvkm_therm_temp_get() interface reports temperatures in one degree Celsius
+ * increments.
+ *
+ * Return: The temperature in millidegrees Celsius, or -ENODEV if temperature
+ * reporting is not supported.
+ */
+int nvkm_therm_temp_millidegree_get(struct nvkm_therm *therm);
int nvkm_therm_fan_sense(struct nvkm_therm *);
int nvkm_therm_cstate(struct nvkm_therm *, int, int);
void nvkm_therm_clkgate_init(struct nvkm_therm *,
diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
index 1c3104d20571..e96355f93ce5 100644
--- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
@@ -428,8 +428,8 @@ nouveau_temp_read(struct device *dev, u32 attr, int
channel, long *val)
case hwmon_temp_input:
if (drm_dev->switch_power_state != DRM_SWITCH_POWER_ON)
return -EINVAL;
- ret = nvkm_therm_temp_get(therm);
- *val = ret < 0 ? ret : (ret * 1000);
+ ret = nvkm_therm_temp_millidegree_get(therm);
+ *val = ret;
break;
case hwmon_temp_max:
*val = therm->attr_get(therm, NVKM_THERM_ATTR_THRS_DOWN_CLK)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/base.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/base.c
index 4a4d1e224126..e655b32c78b8 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/base.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/base.c
@@ -34,6 +34,22 @@ nvkm_therm_temp_get(struct nvkm_therm *therm)
return -ENODEV;
}
+int
+nvkm_therm_temp_millidegree_get(struct nvkm_therm *therm)
+{
+ int ret = -ENODEV;
+
+ if (therm->func->temp_millidegree_get)
+ return therm->func->temp_millidegree_get(therm);
+
+ if (therm->func->temp_get) {
+ ret = therm->func->temp_get(therm);
+ if (ret > 0)
+ ret *= 1000;
+ }
+ return ret;
+}
+
static int
nvkm_therm_update_trip(struct nvkm_therm *therm)
{
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
index 9f0dea3f61dc..4c3c2895a3cb 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
@@ -24,7 +24,7 @@
#include "priv.h"
static int
-gp100_temp_get(struct nvkm_therm *therm)
+gp100_temp_get_raw(struct nvkm_therm *therm)
{
struct nvkm_device *device = therm->subdev.device;
struct nvkm_subdev *subdev = >subdev;
@@ -37,14 +37,35 @@ gp100_temp_get(struct nvkm_therm *therm)
/* device valid */
if (tsensor & 0x2000)
- return (inttemp >> 8);
+ return inttemp;
else
return -ENODEV;
}
+static int
+gp100_temp_millidegree_get(struct nvkm_therm *therm)
+{
+ int raw_temp = gp100_temp_get_raw(therm);
+
+ if (raw_temp < 0)
+ return raw_temp;
+ return raw_temp * 1000 >> 8;
+}
+
+static int
+gp100_temp_get(struct nvkm_therm *therm)
+{
+ int raw_temp = gp100_temp_get_raw(therm);
+
+ if (raw_temp < 0)
+ retu
[PATCH RESEND] lockdown: Allow unprivileged users to see lockdown status
A number of userspace tools, such as systemtap, need a way to see the
current lockdown state so they can gracefully deal with the kernel being
locked down. The state is already exposed in
/sys/kernel/security/lockdown, but is only readable by root. Adjust the
permissions so unprivileged users can read the state.
Fixes: 000d388ed3bb ("security: Add a static lockdown policy LSM")
Cc: Frank Ch. Eigler
Signed-off-by: Jeremy Cline
---
security/lockdown/lockdown.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
index 5a952617a0eba..87cbdc64d272c 100644
--- a/security/lockdown/lockdown.c
+++ b/security/lockdown/lockdown.c
@@ -150,7 +150,7 @@ static int __init lockdown_secfs_init(void)
{
struct dentry *dentry;
- dentry = securityfs_create_file("lockdown", 0600, NULL, NULL,
+ dentry = securityfs_create_file("lockdown", 0644, NULL, NULL,
_ops);
return PTR_ERR_OR_ZERO(dentry);
}
--
2.26.2
Re: [PATCH] lockdown: Allow unprivileged users to see lockdown status
On Sat, Feb 22, 2020 at 03:51:24AM +1100, James Morris wrote:
> On Thu, 20 Feb 2020, Jeremy Cline wrote:
>
> > A number of userspace tools, such as systemtap, need a way to see the
> > current lockdown state so they can gracefully deal with the kernel being
> > locked down. The state is already exposed in
> > /sys/kernel/security/lockdown, but is only readable by root. Adjust the
> > permissions so unprivileged users can read the state.
> >
> > Fixes: 000d388ed3bb ("security: Add a static lockdown policy LSM")
> > Cc: Frank Ch. Eigler
> > Signed-off-by: Jeremy Cline
>
> Looks fine to me, any objection from Matthew or others?
>
Can we take resounding silence as no objections?
- Jeremy
> > ---
> > security/lockdown/lockdown.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
> > index 5a952617a0eb..87cbdc64d272 100644
> > --- a/security/lockdown/lockdown.c
> > +++ b/security/lockdown/lockdown.c
> > @@ -150,7 +150,7 @@ static int __init lockdown_secfs_init(void)
> > {
> > struct dentry *dentry;
> >
> > - dentry = securityfs_create_file("lockdown", 0600, NULL, NULL,
> > + dentry = securityfs_create_file("lockdown", 0644, NULL, NULL,
> > _ops);
> > return PTR_ERR_OR_ZERO(dentry);
> > }
> >
>
> --
> James Morris
>
>
[PATCH] docs: kmemleak: DEBUG_KMEMLEAK_EARLY_LOG_SIZE changed names
Commit c5665868183f ("mm: kmemleak: use the memory pool for early
allocations") renamed CONFIG_DEBUG_KMEMLEAK_EARLY_LOG_SIZE to
CONFIG_DEBUG_KMEMLEAK_MEM_POOL_SIZE. Update the documentation reference
to reflect that.
Signed-off-by: Jeremy Cline
---
Documentation/dev-tools/kmemleak.rst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/dev-tools/kmemleak.rst
b/Documentation/dev-tools/kmemleak.rst
index 3621cd5e1eef..3a289e8a1d12 100644
--- a/Documentation/dev-tools/kmemleak.rst
+++ b/Documentation/dev-tools/kmemleak.rst
@@ -69,7 +69,7 @@ the kernel command line.
Memory may be allocated or freed before kmemleak is initialised and
these actions are stored in an early log buffer. The size of this buffer
-is configured via the CONFIG_DEBUG_KMEMLEAK_EARLY_LOG_SIZE option.
+is configured via the CONFIG_DEBUG_KMEMLEAK_MEM_POOL_SIZE option.
If CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF are enabled, the kmemleak is
disabled by default. Passing ``kmemleak=on`` on the kernel command
--
2.21.0
[PATCH] arm64: Fix reference to docs for ARM64_TAGGED_ADDR_ABI
The referenced file does not exist, but tagged-address-abi.rst does. Signed-off-by: Jeremy Cline --- arch/arm64/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 6ae6ad8a4db0..8960310b4f64 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -1135,7 +1135,7 @@ config ARM64_TAGGED_ADDR_ABI When this option is enabled, user applications can opt in to a relaxed ABI via prctl() allowing tagged addresses to be passed to system calls as pointer arguments. For details, see - Documentation/arm64/tagged-address-abi.txt. + Documentation/arm64/tagged-address-abi.rst. menuconfig COMPAT bool "Kernel support for 32-bit EL0" -- 2.21.0
Re: [PATCH v2] HID: input: fix a4tech horizontal wheel custom usage
On Fri, Jun 14, 2019 at 03:36:03PM +0200, Benjamin Tissoires wrote:
> Hi Wolfgang,
>
> On Thu, Jun 13, 2019 at 1:49 PM Wolfgang Bauer wrote:
> >
> > On Tuesday, 11. Juni 2019, 16:42:37 Benjamin Tissoires wrote:
> > > On Tue, Jun 11, 2019 at 2:13 PM Nicolas Saenz Julienne
> > >
> > > wrote:
> > > > NOTE: I CC'd Wolfgang as he's the one who can test this.
> > >
> > > I'll wait for Wolfram to confirm that the patch works before pushing then.
> >
> > My name is Wolfgang, not Wolfram... ;-)
>
> ouch, sorry for that (I am more used to talk to the I2C maintainer apparently)
>
> > But never mind.
> >
> > I tested the patch meanwhile on top of kernel 5.2.rc4, where the mouse wheel
> > actually worked.
>
> Actually, I am a little bit lost here.
>
> The patch mentions a fix of c01908a14bf73, which is in 5.1 final.
> So if your mouse works in 5.2.rc4, I am not sure how
> HID-a4tech-fix-horizontal-scrolling.patch could break it.
>
> Could you be slightly more specific in what "works" and what doesn't?
>
For what it's worth, at least one user has hit this issue in Fedora
starting on stable kernel v5.1.17[0] which has commit abf82e8f7e9a
("HID: a4tech: fix horizontal scrolling"). Applying this patch on top of
v5.1.18 fixed the issue for them.
[0] https://bugzilla.redhat.com/show_bug.cgi?id=1730762
Regards,
Jeremy
Re: [PATCH v2] KEYS: Make use of platform keyring for module signature verify
Hi,
On Thu, Apr 25, 2019 at 07:55:50AM -0400, Mimi Zohar wrote:
> On Wed, 2019-04-24 at 14:33 +, Robert Holmes wrote:
> > This patch completes commit 278311e417be ("kexec, KEYS: Make use of
> > platform keyring for signature verify") which, while adding the
> > platform keyring for bzImage verification, neglected to also add
> > this keyring for module verification.
> >
> > As such, kernel modules signed with keys from the MokList variable
> > were not successfully verified.
>
> Using the platform keyring keys for verifying kernel modules was not
> neglected, but rather intentional. This patch description should
> clearly explain the reason for needing to verify kernel module
> signatures based on the pre-boot keys. (Hint: verifying kernel
> modules based on the pre-boot keys was previously rejected.)
So the background for this patch is that Fedora, which carries the
lockdown patch set, recently regressed[0] with respect to user-signed
modules. Previously, we carried a patch that added all the pre-boot keys
to the secondary keyring. That way users could add a machine owner key
and use secure boot and lockdown with their self-signed 3rd party modules.
Since the pre-boot keys are now loaded into the platform keyring, I
suggested that Robert submit the patch upstream, but since the lockdown
patches aren't upstream perhaps it doesn't make much sense to pick this
up and Fedora should continue carrying it.
[0] https://bugzilla.redhat.com/show_bug.cgi?id=1701096
Regards,
Jeremy
> >
> > Signed-off-by: Robert Holmes
> > Cc: linux-integr...@vger.kernel.org
> > Cc: keyri...@vger.kernel.org
> > Cc: sta...@vger.kernel.org
> > ---
> > kernel/module_signing.c | 16
> > 1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/module_signing.c b/kernel/module_signing.c
> > index 6b9a926fd86b..cf94220e9154 100644
> > --- a/kernel/module_signing.c
> > +++ b/kernel/module_signing.c
> > @@ -49,6 +49,7 @@ int mod_verify_sig(const void *mod, struct load_info
> > *info)
> > {
> > struct module_signature ms;
> > size_t sig_len, modlen = info->len;
> > + int ret;
> >
> > pr_devel("==>%s(,%zu)\n", __func__, modlen);
> >
> > @@ -82,8 +83,15 @@ int mod_verify_sig(const void *mod, struct load_info
> > *info)
> > return -EBADMSG;
> > }
> >
> > - return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
> > - VERIFY_USE_SECONDARY_KEYRING,
> > - VERIFYING_MODULE_SIGNATURE,
> > - NULL, NULL);
> > + ret = verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
> > +VERIFY_USE_SECONDARY_KEYRING,
> > +VERIFYING_MODULE_SIGNATURE,
> > +NULL, NULL);
> > + if (ret == -ENOKEY && IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING)) {
> > + ret = verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
> > +VERIFY_USE_PLATFORM_KEYRING,
> > +VERIFYING_MODULE_SIGNATURE,
> > +NULL, NULL);
> > + }
> > + return ret;
> > }
>
Re: [PATCH] Bluetooth: hci_ldisc: Postpone HCI_UART_PROTO_READY bit set in hci_uart_set_proto()
On Sat, Feb 23, 2019 at 12:33:27PM +0800, Kefeng Wang wrote:
> task A:task B:
> hci_uart_set_proto flush_to_ldisc
> - p->open(hu) -> h5_open //alloc h5 - receive_buf
> - set_bit HCI_UART_PROTO_READY - tty_port_default_receive_buf
> - hci_uart_register_dev - tty_ldisc_receive_buf
> - hci_uart_tty_receive
> - test_bit HCI_UART_PROTO_READY
> - h5_recv
> - clear_bit HCI_UART_PROTO_READY while() {
> - p->open(hu) -> h5_close //free h5
I think commit 32a7b4cbe93b ("Bluetooth: hci_ldisc: Initialize hci_dev
before open()") in made this worse by inadvertently changing this from
- clear_bit HCI_UART_PROTO_READY
- p->close(hu) -> h5_close //free h5
to
- p->close(hu) -> h5_close //free h5
- clear_bit HCI_UART_PROTO_READY
because the close call got moved into hci_uart_register_dev(). I guess
that made the race window sufficiently wide to be easily reproducible,
but it looks like it was present even before that commit.
> - h5_rx_3wire_hdr
> - h5_reset() //use-after-free
> }
>
> It could use ioctl to set hci uart proto, but there is
> a use-after-free issue when hci_uart_register_dev() fail in
> hci_uart_set_proto(), see stack above, fix this by setting
> HCI_UART_PROTO_READY bit only when hci_uart_register_dev()
> return success.
>
> Reported-by: syzbot+899a33dc0fa0dbaf0...@syzkaller.appspotmail.com
> Signed-off-by: Kefeng Wang
> ---
> drivers/bluetooth/hci_ldisc.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index 4918fefc4a6f..9562e72c1ae5 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -696,14 +696,13 @@ static int hci_uart_set_proto(struct hci_uart *hu, int
> id)
> return -EPROTONOSUPPORT;
>
> hu->proto = p;
> - set_bit(HCI_UART_PROTO_READY, >flags);
>
> err = hci_uart_register_dev(hu);
> if (err) {
> - clear_bit(HCI_UART_PROTO_READY, >flags);
> return err;
> }
>
> + set_bit(HCI_UART_PROTO_READY, >flags);
> return 0;
> }
>
> --
> 2.20.1
>
For what it's worth:
Reviewed-by: Jeremy Cline
Re: [PATCH] proc: update i_atime when reading files
On Fri, Feb 22, 2019 at 08:37:42AM +0300, Alexey Dobriyan wrote:
> On Thu, Feb 21, 2019 at 11:37:14AM -0500, Jeremy Cline wrote:
> > Prior to commit 1da4d377f943 ("proc: revalidate misc dentries"), the
> > access, modify, and change times of files in /proc were just the current
> > time.
>
> Ehh, actually no. Doing
>
> $(which sleep) infinity
> will sabotage atime updates because dentry and inode will be pinned in
> caches.
>
> "revalidate misc denries" commit simply makes the effect (much) more
> visible by making objects stay in caches for longer.
Indeed. It wasn't my intention to imply there's anything wrong with that
commit, just that that's what caused this apparent change in behavior
for users. In the "common" case when something hasn't pinned the dentry
and inode what users saw was the current time.
>
> > Now the mtime and ctime values change mostly as a user would
> > expect, but the atime isn't updated when the file read. This patch
> > updates the access time of /proc files when they are read.
>
> > rv = read(file, buf, count, ppos);
> > + if (rv >= 0)
> > + inode->i_atime = current_time(inode);
> > + }
>
> Maybe it should be done given /proc is virtual so there are no concerns
> about scheduling writes noone cares about to the filesystem.
Sorry, maybe I've not had enough coffee yet, but I don't understand this
sentence.
Thanks,
Jeremy
[PATCH] proc: update i_atime when reading files
Prior to commit 1da4d377f943 ("proc: revalidate misc dentries"), the
access, modify, and change times of files in /proc were just the current
time. Now the mtime and ctime values change mostly as a user would
expect, but the atime isn't updated when the file read. This patch
updates the access time of /proc files when they are read.
Reported-by: David Both
Signed-off-by: Jeremy Cline
---
fs/proc/inode.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index da649ccd6804..34d8603b9aa1 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -221,12 +221,17 @@ static loff_t proc_reg_llseek(struct file *file, loff_t
offset, int whence)
static ssize_t proc_reg_read(struct file *file, char __user *buf, size_t
count, loff_t *ppos)
{
ssize_t (*read)(struct file *, char __user *, size_t, loff_t *);
- struct proc_dir_entry *pde = PDE(file_inode(file));
+ struct inode *inode = file_inode(file);
+ struct proc_dir_entry *pde = PDE(inode);
ssize_t rv = -EIO;
if (use_pde(pde)) {
read = pde->proc_fops->read;
- if (read)
+ if (read) {
rv = read(file, buf, count, ppos);
+ if (rv >= 0)
+ inode->i_atime = current_time(inode);
+ }
+
unuse_pde(pde);
}
return rv;
--
2.20.1
[PATCH] Bluetooth: hci_ldisc: Initialize hci_dev before open()
The hci_dev struct hdev is referenced in work queues and timers started
by open() in some protocols. This creates a race between the
initialization function and the work or timer which can result hdev
being dereferenced while it is still null.
The syzbot report contains a reliable reproducer which causes a null
pointer dereference of hdev in hci_uart_write_work() by making the
memory allocation for hdev fail.
To fix this, ensure hdev is valid from before calling a protocol's
open() until after calling a protocol's close().
Reported-by: syzbot+257790c15bcdef6fe...@syzkaller.appspotmail.com
Signed-off-by: Jeremy Cline
---
drivers/bluetooth/hci_ldisc.c | 21 ++---
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index fbf7b4df23ab..4918fefc4a6f 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -207,11 +207,11 @@ void hci_uart_init_work(struct work_struct *work)
err = hci_register_dev(hu->hdev);
if (err < 0) {
BT_ERR("Can't register HCI device");
+ clear_bit(HCI_UART_PROTO_READY, >flags);
+ hu->proto->close(hu);
hdev = hu->hdev;
hu->hdev = NULL;
hci_free_dev(hdev);
- clear_bit(HCI_UART_PROTO_READY, >flags);
- hu->proto->close(hu);
return;
}
@@ -616,6 +616,7 @@ static void hci_uart_tty_receive(struct tty_struct *tty,
const u8 *data,
static int hci_uart_register_dev(struct hci_uart *hu)
{
struct hci_dev *hdev;
+ int err;
BT_DBG("");
@@ -659,11 +660,22 @@ static int hci_uart_register_dev(struct hci_uart *hu)
else
hdev->dev_type = HCI_PRIMARY;
+ /* Only call open() for the protocol after hdev is fully initialized as
+* open() (or a timer/workqueue it starts) may attempt to reference it.
+*/
+ err = hu->proto->open(hu);
+ if (err) {
+ hu->hdev = NULL;
+ hci_free_dev(hdev);
+ return err;
+ }
+
if (test_bit(HCI_UART_INIT_PENDING, >hdev_flags))
return 0;
if (hci_register_dev(hdev) < 0) {
BT_ERR("Can't register HCI device");
+ hu->proto->close(hu);
hu->hdev = NULL;
hci_free_dev(hdev);
return -ENODEV;
@@ -683,17 +695,12 @@ static int hci_uart_set_proto(struct hci_uart *hu, int id)
if (!p)
return -EPROTONOSUPPORT;
- err = p->open(hu);
- if (err)
- return err;
-
hu->proto = p;
set_bit(HCI_UART_PROTO_READY, >flags);
err = hci_uart_register_dev(hu);
if (err) {
clear_bit(HCI_UART_PROTO_READY, >flags);
- p->close(hu);
return err;
}
--
2.20.1
[regression] mei: me: allow runtime pm for platform with D0i3
Hi folks,
Fedora has received a number of reports of e1000e-driven ethernet ports
negotiating 10Mbps links if the cable is disconnected and reconnected
after boot on a number of laptops, including the Lenovo T480s[0].
Kamil git-bisected the issue to commit cc365dcf0e56 ("mei: me: allow
runtime pm for platform with D0i3") and has confirmed that reverting
that commit in v4.20 fixes the issue.
[0] https://bugzilla.redhat.com/show_bug.cgi?id=1627816
Regards,
Jeremy
Re: [PATCH 4.19 061/110] ACPICA: AML interpreter: add region addresses in global list during initialization
Hi folks, On 11/29/18 9:12 AM, Greg Kroah-Hartman wrote: > 4.19-stable review patch. If anyone has any objections, please let me know. > > -- > > From: Erik Schmauss > > commit 4abb951b73ff0a8a979113ef185651aa3c8da19b upstream. > > The table load process omitted adding the operation region address > range to the global list. This omission is problematic because the OS > queries the global list to check for address range conflicts before > deciding which drivers to load. This commit may result in warning > messages that look like the following: > > [7.871761] ACPI Warning: system_IO range 0x0428-0x042F conflicts > with op_region 0x0400-0x047F (\PMIO) (20180531/utaddress-213) > [7.871769] ACPI: If an ACPI driver is available for this device, you > should use it instead of the native driver > > However, these messages do not signify regressions. It is a result of > properly adding address ranges within the global address list> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=200011 > Tested-by: Jean-Marc Lenoir > Signed-off-by: Erik Schmauss > Cc: All applicable > Signed-off-by: Rafael J. Wysocki > Cc: Jean Delvare > Signed-off-by: Greg Kroah-Hartman > > --- > drivers/acpi/acpica/dsopcode.c |4 > 1 file changed, 4 insertions(+) > > --- a/drivers/acpi/acpica/dsopcode.c > +++ b/drivers/acpi/acpica/dsopcode.c > @@ -417,6 +417,10 @@ acpi_ds_eval_region_operands(struct acpi > ACPI_FORMAT_UINT64(obj_desc->region.address), > obj_desc->region.length)); > > + status = acpi_ut_add_address_range(obj_desc->region.space_id, > +obj_desc->region.address, > +obj_desc->region.length, node); > + > /* Now the address and length are valid for this opregion */ > > obj_desc->region.flags |= AOPOBJ_DATA_VALID; > > Fedora has received a couple of reports[0] of boot failures on v4.19 kernels: kernel: BUG: unable to handle kernel paging request at 003600b6 kernel: PGD 0 P4D 0 kernel: Oops: [#1] SMP NOPTI kernel: CPU: 0 PID: 108 Comm: kworker/0:3 Not tainted 4.19.8-300.fc29.x86_64 #1 kernel: Hardware name: Hewlett-Packard HP Compaq 6715b/30C2, BIOS 68YTT Ver. F.07 07/16/2007 kernel: Workqueue: events work_for_cpu_fn kernel: RIP: 0010:acpi_ns_build_normalized_path+0x76/0x108 kernel: Code: 31 c0 48 85 ff 74 de 31 d2 49 89 f8 31 c0 4c 39 05 ff 5b 57 01 74 61 4c 39 c7 74 0e 39 c2 76 08 41 89 c1 42 c6 04 0e 2e ff c0 <45> 8b 48 0c 41 89 ca 44 89 4c 24 04 45 31 c9 46 8a 5c 0c 07 45 84 kernel: RSP: 0018:b68740a1fce0 EFLAGS: 00010207 kernel: RAX: 000a RBX: 0001 RCX: 0001 kernel: RDX: RSI: RDI: 9dd2360e7168 kernel: RBP: R08: 003600aa R09: fffc kernel: R10: R11: 00d2 R12: 8200 kernel: R13: 9dd2360e7168 R14: 9dd2706980c0 R15: 0001 kernel: FS: () GS:9dd275c0() knlGS: kernel: CS: 0010 DS: ES: CR0: 80050033 kernel: CR2: 003600b6 CR3: 7137e000 CR4: 06f0 kernel: Call Trace: kernel: acpi_ns_get_normalized_pathname+0x1f/0x6d kernel: acpi_ut_check_address_range+0x8f/0x118 kernel: acpi_check_address_range+0x40/0x59 kernel: acpi_check_resource_conflict+0x3a/0x50 kernel: acpi_check_region+0x61/0x80 kernel: piix4_probe+0x144/0x658 [i2c_piix4] kernel: local_pci_probe+0x41/0x90 kernel: work_for_cpu_fn+0x16/0x20 kernel: process_one_work+0x1a1/0x3a0 kernel: worker_thread+0x30/0x380 kernel: ? pwq_unbound_release_workfn+0xd0/0xd0 kernel: kthread+0x112/0x130 kernel: ? kthread_create_worker_on_cpu+0x70/0x70 kernel: ret_from_fork+0x35/0x40 kernel: Modules linked in: i2c_piix4(+) parport_pc wmi tpm_infineon parport ssb hp_accel lis3lv02d mmc_core input_polldev video radeon i2c_algo_bit drm_kms_helper ttm drm firewire_ohci firewire_core ata_generic serio_raw pata_acpi yenta_socket crc_itu_t pata_atiixp kernel: CR2: 003600b6 kernel: ---[ end trace 9c88eedaaeca7db0 ]--- I reverted this patch on v4.19.8 and at least one user (Michael, Cc'd) reported that fixed the regression. This patch appears to have been included in v4.19.2 (commit 22083c028d0b), reverted in v4.19.3 (commit 8ef305fbc50d), and finally included again here in v4.19.6 (commit 87403e35bc56). I couldn't find a discussion about bringing it back, so maybe it happened accidentally? [0] https://bugzilla.redhat.com/show_bug.cgi?id=1659225 Thanks, Jeremy
Re: [PATCH 1/2] scripts/spdxcheck.py: Always open files in binary mode
On Thu, Dec 13, 2018 at 08:37:08AM +0100, Uwe Kleine-König wrote:
> On Wed, Dec 12, 2018 at 01:14:10PM -0500, Jeremy Cline wrote:
> > Hi,
> >
> > On Wed, Dec 12, 2018 at 02:12:09PM +0100, Thierry Reding wrote:
> > > From: Thierry Reding
> > >
> > > The spdxcheck script currently falls over when confronted with a binary
> > > file (such as Documentation/logo.gif). To avoid that, always open files
> > > in binary mode and decode line-by-line, ignoring encoding errors.
>
> I suggest pointing out that the breakage only happens with python3 and
> results in a UnicodeDecodeError.
>
> > > One tricky case is when piping data into the script and reading it from
> > > standard input. By default, standard input will be opened in text mode,
> > > so we need to reopen it in binary mode.
> > >
> > > Signed-off-by: Thierry Reding
> > > ---
> > > scripts/spdxcheck.py | 6 --
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/scripts/spdxcheck.py b/scripts/spdxcheck.py
> > > index 5056fb3b897d..e559c6294c39 100755
> > > --- a/scripts/spdxcheck.py
> > > +++ b/scripts/spdxcheck.py
> > > @@ -168,6 +168,7 @@ class id_parser(object):
> > > self.curline = 0
> > > try:
> > > for line in fd:
> > > +line = line.decode(locale.getpreferredencoding(False),
> > > errors='ignore')
> > > self.curline += 1
> > > if self.curline > maxlines:
> > > break
> > > @@ -249,12 +250,13 @@ if __name__ == '__main__':
> > >
> > > try:
> > > if len(args.path) and args.path[0] == '-':
> > > -parser.parse_lines(sys.stdin, args.maxlines, '-')
> > > +stdin = os.fdopen(sys.stdin.fileno(), 'rb')
> > > +parser.parse_lines(stdin, args.maxlines, '-')
> > > else:
> > > if args.path:
> > > for p in args.path:
> > > if os.path.isfile(p):
> > > -parser.parse_lines(open(p), args.maxlines, p)
> > > +parser.parse_lines(open(p, 'rb'), args.maxlines,
> > > p)
> > > elif os.path.isdir(p):
> > >
> > > scan_git_subtree(repo.head.reference.commit.tree, p)
> > > else:
> > > --
> > > 2.19.1
> > >
> >
> > It might be worth noting this fixes commit 6f4d29df66ac
> > ("scripts/spdxcheck.py: make python3 compliant") and also Cc this for
> > stable since 6f4d29df66ac got backported to v4.19. While that commit
> > did indeed make the script work with Python 3 for piping data, it broke
> > Python 2 and made its way to stable.
>
> It didn't break for me. Can you provide details about how and when it
> broke for you?
I was wrong about it being Python 2 that broke, sorry about that.
6f4d29df66ac broke Python 3 when you run it against a sub-tree because
scan_git_tree() opens the files in binary mode, but then find is run
with a text string:
$ python3 scripts/spdxcheck.py net/
FAIL: argument should be integer or bytes-like object, not 'str'
Traceback (most recent call last):
File "scripts/spdxcheck.py", line 259, in
scan_git_subtree(repo.head.reference.commit.tree, p)
File "scripts/spdxcheck.py", line 211, in scan_git_subtree
scan_git_tree(tree)
File "scripts/spdxcheck.py", line 206, in scan_git_tree
parser.parse_lines(fd, args.maxlines, el.path)
File "scripts/spdxcheck.py", line 175, in parse_lines
if line.find("SPDX-License-Identifier:") < 0:
TypeError: argument should be integer or bytes-like object, not 'str'
The reason I opened things in binary mode when I started adding Python 3
support was because not all files were valid UTF-8 (and some were
binary) so I decoded the text line-by-line and ignored any decoding
errors for simplicity's sake.
Regards,
Jeremy
Re: [PATCH 1/2] scripts/spdxcheck.py: Always open files in binary mode
Hi,
On Wed, Dec 12, 2018 at 02:12:09PM +0100, Thierry Reding wrote:
> From: Thierry Reding
>
> The spdxcheck script currently falls over when confronted with a binary
> file (such as Documentation/logo.gif). To avoid that, always open files
> in binary mode and decode line-by-line, ignoring encoding errors.
>
> One tricky case is when piping data into the script and reading it from
> standard input. By default, standard input will be opened in text mode,
> so we need to reopen it in binary mode.
>
> Signed-off-by: Thierry Reding
> ---
> scripts/spdxcheck.py | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/spdxcheck.py b/scripts/spdxcheck.py
> index 5056fb3b897d..e559c6294c39 100755
> --- a/scripts/spdxcheck.py
> +++ b/scripts/spdxcheck.py
> @@ -168,6 +168,7 @@ class id_parser(object):
> self.curline = 0
> try:
> for line in fd:
> +line = line.decode(locale.getpreferredencoding(False),
> errors='ignore')
> self.curline += 1
> if self.curline > maxlines:
> break
> @@ -249,12 +250,13 @@ if __name__ == '__main__':
>
> try:
> if len(args.path) and args.path[0] == '-':
> -parser.parse_lines(sys.stdin, args.maxlines, '-')
> +stdin = os.fdopen(sys.stdin.fileno(), 'rb')
> +parser.parse_lines(stdin, args.maxlines, '-')
> else:
> if args.path:
> for p in args.path:
> if os.path.isfile(p):
> -parser.parse_lines(open(p), args.maxlines, p)
> +parser.parse_lines(open(p, 'rb'), args.maxlines, p)
> elif os.path.isdir(p):
> scan_git_subtree(repo.head.reference.commit.tree, p)
> else:
> --
> 2.19.1
>
It might be worth noting this fixes commit 6f4d29df66ac
("scripts/spdxcheck.py: make python3 compliant") and also Cc this for
stable since 6f4d29df66ac got backported to v4.19. While that commit
did indeed make the script work with Python 3 for piping data, it broke
Python 2 and made its way to stable.
Reviewed-by: Jeremy Cline
Regards,
Jeremy
Re: Regression: very quiet speakers on Thinkpad T570s
On 12/1/18 9:44 AM, Takashi Iwai wrote:
> On Fri, 30 Nov 2018 17:51:33 +0100,
> Jeremy Cline wrote:
>>
>> On 11/30/18 11:00 AM, Takashi Iwai wrote:
>>> On Fri, 30 Nov 2018 15:49:17 +0100,
>>> Jeremy Cline wrote:
>>>>
>>>> Hi,
>>>>
>>>> Some folks have reported on the Fedora bug tracker[0] that the laptop
>>>> speaker volume is very low on the Thinkpad T570 when running a kernel
>>>> that includes commit 61fcf8ece9b6 ("ALSA: hda/realtek - Enable Thinkpad
>>>> Dock device for ALC298 platform").
>>>>
>>>> alsa-info.sh from v4.15.4 (just before commit 61fcf8ece9b6 arrived in
>>>> stable) and v4.19.4 with the issue present are attached to the bugzilla.
>>>> I've also Cc'd Tim, who uploaded them and has the laptop in question.
>>>>
>>>> [0] https://bugzilla.redhat.com/show_bug.cgi?id=1554304
>>>
>>> Could you pinpoint which kernel version started showing the
>>> regression, at least? The diffs are fairly wide between 4.15 and
>>> 4.19.
>>
>> Ah, sorry for not being more clear. The regression appears to be
>> introduced by commit 61fcf8ece9b6, which got backported to v4.15.5
>> because it addressed a bug with the dock[0]. v4.19.4 with that commit
>> reverted works, according to the bug reporter.
>>
>> [0] https://bugzilla.kernel.org/show_bug.cgi?id=195161
>
> OK, then this seems to be the change of DAC assignment.
>
> Maybe the hardware has some implicit assumption of NID 0x03 bound with
> the speaker pin. Below is a patch for fixing the pin / DAC mapping.
> Please give it a try.
>
>
> thanks,
>
> Takashi
>
> ---
> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> index 06f93032d0cc..50bc2e97d799 100644
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -4988,9 +4988,14 @@ static void alc_fixup_tpt470_dock(struct hda_codec
> *codec,
> { 0x19, 0x21a11010 }, /* dock mic */
> { }
> };
> + static hda_nid_t preferred_pairs[] = {
> + 0x14, 0x03, 0x17, 0x02, 0x21, 0x02,
> + 0
> + };
> struct alc_spec *spec = codec->spec;
>
> if (action == HDA_FIXUP_ACT_PRE_PROBE) {
> + spec->gen.preferred_dacs = preferred_pairs;
> spec->parse_flags = HDA_PINCFG_NO_HP_FIXUP;
> snd_hda_apply_pincfgs(codec, pincfgs);
> } else if (action == HDA_FIXUP_ACT_INIT) {
>
That patch does indeed fix the problem according to the original
reporter.
Thanks,
Jeremy
Re: Regression: very quiet speakers on Thinkpad T570s
On 12/1/18 9:44 AM, Takashi Iwai wrote:
> On Fri, 30 Nov 2018 17:51:33 +0100,
> Jeremy Cline wrote:
>>
>> On 11/30/18 11:00 AM, Takashi Iwai wrote:
>>> On Fri, 30 Nov 2018 15:49:17 +0100,
>>> Jeremy Cline wrote:
>>>>
>>>> Hi,
>>>>
>>>> Some folks have reported on the Fedora bug tracker[0] that the laptop
>>>> speaker volume is very low on the Thinkpad T570 when running a kernel
>>>> that includes commit 61fcf8ece9b6 ("ALSA: hda/realtek - Enable Thinkpad
>>>> Dock device for ALC298 platform").
>>>>
>>>> alsa-info.sh from v4.15.4 (just before commit 61fcf8ece9b6 arrived in
>>>> stable) and v4.19.4 with the issue present are attached to the bugzilla.
>>>> I've also Cc'd Tim, who uploaded them and has the laptop in question.
>>>>
>>>> [0] https://bugzilla.redhat.com/show_bug.cgi?id=1554304
>>>
>>> Could you pinpoint which kernel version started showing the
>>> regression, at least? The diffs are fairly wide between 4.15 and
>>> 4.19.
>>
>> Ah, sorry for not being more clear. The regression appears to be
>> introduced by commit 61fcf8ece9b6, which got backported to v4.15.5
>> because it addressed a bug with the dock[0]. v4.19.4 with that commit
>> reverted works, according to the bug reporter.
>>
>> [0] https://bugzilla.kernel.org/show_bug.cgi?id=195161
>
> OK, then this seems to be the change of DAC assignment.
>
> Maybe the hardware has some implicit assumption of NID 0x03 bound with
> the speaker pin. Below is a patch for fixing the pin / DAC mapping.
> Please give it a try.
>
>
> thanks,
>
> Takashi
>
> ---
> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> index 06f93032d0cc..50bc2e97d799 100644
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -4988,9 +4988,14 @@ static void alc_fixup_tpt470_dock(struct hda_codec
> *codec,
> { 0x19, 0x21a11010 }, /* dock mic */
> { }
> };
> + static hda_nid_t preferred_pairs[] = {
> + 0x14, 0x03, 0x17, 0x02, 0x21, 0x02,
> + 0
> + };
> struct alc_spec *spec = codec->spec;
>
> if (action == HDA_FIXUP_ACT_PRE_PROBE) {
> + spec->gen.preferred_dacs = preferred_pairs;
> spec->parse_flags = HDA_PINCFG_NO_HP_FIXUP;
> snd_hda_apply_pincfgs(codec, pincfgs);
> } else if (action == HDA_FIXUP_ACT_INIT) {
>
That patch does indeed fix the problem according to the original
reporter.
Thanks,
Jeremy
Re: Regression: very quiet speakers on Thinkpad T570s
On 11/30/18 11:00 AM, Takashi Iwai wrote:
> On Fri, 30 Nov 2018 15:49:17 +0100,
> Jeremy Cline wrote:
>>
>> Hi,
>>
>> Some folks have reported on the Fedora bug tracker[0] that the laptop
>> speaker volume is very low on the Thinkpad T570 when running a kernel
>> that includes commit 61fcf8ece9b6 ("ALSA: hda/realtek - Enable Thinkpad
>> Dock device for ALC298 platform").
>>
>> alsa-info.sh from v4.15.4 (just before commit 61fcf8ece9b6 arrived in
>> stable) and v4.19.4 with the issue present are attached to the bugzilla.
>> I've also Cc'd Tim, who uploaded them and has the laptop in question.
>>
>> [0] https://bugzilla.redhat.com/show_bug.cgi?id=1554304
>
> Could you pinpoint which kernel version started showing the
> regression, at least? The diffs are fairly wide between 4.15 and
> 4.19.
Ah, sorry for not being more clear. The regression appears to be
introduced by commit 61fcf8ece9b6, which got backported to v4.15.5
because it addressed a bug with the dock[0]. v4.19.4 with that commit
reverted works, according to the bug reporter.
[0] https://bugzilla.kernel.org/show_bug.cgi?id=195161
Regards,
Jeremy
Re: Regression: very quiet speakers on Thinkpad T570s
On 11/30/18 11:00 AM, Takashi Iwai wrote:
> On Fri, 30 Nov 2018 15:49:17 +0100,
> Jeremy Cline wrote:
>>
>> Hi,
>>
>> Some folks have reported on the Fedora bug tracker[0] that the laptop
>> speaker volume is very low on the Thinkpad T570 when running a kernel
>> that includes commit 61fcf8ece9b6 ("ALSA: hda/realtek - Enable Thinkpad
>> Dock device for ALC298 platform").
>>
>> alsa-info.sh from v4.15.4 (just before commit 61fcf8ece9b6 arrived in
>> stable) and v4.19.4 with the issue present are attached to the bugzilla.
>> I've also Cc'd Tim, who uploaded them and has the laptop in question.
>>
>> [0] https://bugzilla.redhat.com/show_bug.cgi?id=1554304
>
> Could you pinpoint which kernel version started showing the
> regression, at least? The diffs are fairly wide between 4.15 and
> 4.19.
Ah, sorry for not being more clear. The regression appears to be
introduced by commit 61fcf8ece9b6, which got backported to v4.15.5
because it addressed a bug with the dock[0]. v4.19.4 with that commit
reverted works, according to the bug reporter.
[0] https://bugzilla.kernel.org/show_bug.cgi?id=195161
Regards,
Jeremy
Regression: very quiet speakers on Thinkpad T570s
Hi,
Some folks have reported on the Fedora bug tracker[0] that the laptop
speaker volume is very low on the Thinkpad T570 when running a kernel
that includes commit 61fcf8ece9b6 ("ALSA: hda/realtek - Enable Thinkpad
Dock device for ALC298 platform").
alsa-info.sh from v4.15.4 (just before commit 61fcf8ece9b6 arrived in
stable) and v4.19.4 with the issue present are attached to the bugzilla.
I've also Cc'd Tim, who uploaded them and has the laptop in question.
[0] https://bugzilla.redhat.com/show_bug.cgi?id=1554304
Regards,
Jeremy
Regression: very quiet speakers on Thinkpad T570s
Hi,
Some folks have reported on the Fedora bug tracker[0] that the laptop
speaker volume is very low on the Thinkpad T570 when running a kernel
that includes commit 61fcf8ece9b6 ("ALSA: hda/realtek - Enable Thinkpad
Dock device for ALC298 platform").
alsa-info.sh from v4.15.4 (just before commit 61fcf8ece9b6 arrived in
stable) and v4.19.4 with the issue present are attached to the bugzilla.
I've also Cc'd Tim, who uploaded them and has the laptop in question.
[0] https://bugzilla.redhat.com/show_bug.cgi?id=1554304
Regards,
Jeremy
[PATCH] ALSA: hda - Add mic quirk for the Lenovo G50-30 (17aa:3905)
The Lenovo G50-30, like other G50 models, has a Conexant codec that
requires a quirk for its inverted stereo dmic.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1249364
Reported-by: Alexander Ploumistos
Tested-by: Alexander Ploumistos
Cc: sta...@vger.kernel.org
Signed-off-by: Jeremy Cline
---
sound/pci/hda/patch_conexant.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
index 5592557fe50e..950e02e71766 100644
--- a/sound/pci/hda/patch_conexant.c
+++ b/sound/pci/hda/patch_conexant.c
@@ -943,6 +943,7 @@ static const struct snd_pci_quirk cxt5066_fixups[] = {
SND_PCI_QUIRK(0x17aa, 0x21da, "Lenovo X220", CXT_PINCFG_LENOVO_TP410),
SND_PCI_QUIRK(0x17aa, 0x21db, "Lenovo X220-tablet",
CXT_PINCFG_LENOVO_TP410),
SND_PCI_QUIRK(0x17aa, 0x38af, "Lenovo IdeaPad Z560",
CXT_FIXUP_MUTE_LED_EAPD),
+ SND_PCI_QUIRK(0x17aa, 0x3905, "Lenovo G50-30", CXT_FIXUP_STEREO_DMIC),
SND_PCI_QUIRK(0x17aa, 0x390b, "Lenovo G50-80", CXT_FIXUP_STEREO_DMIC),
SND_PCI_QUIRK(0x17aa, 0x3975, "Lenovo U300s", CXT_FIXUP_STEREO_DMIC),
SND_PCI_QUIRK(0x17aa, 0x3977, "Lenovo IdeaPad U310",
CXT_FIXUP_STEREO_DMIC),
--
2.19.1
[PATCH] ALSA: hda - Add mic quirk for the Lenovo G50-30 (17aa:3905)
The Lenovo G50-30, like other G50 models, has a Conexant codec that
requires a quirk for its inverted stereo dmic.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1249364
Reported-by: Alexander Ploumistos
Tested-by: Alexander Ploumistos
Cc: sta...@vger.kernel.org
Signed-off-by: Jeremy Cline
---
sound/pci/hda/patch_conexant.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
index 5592557fe50e..950e02e71766 100644
--- a/sound/pci/hda/patch_conexant.c
+++ b/sound/pci/hda/patch_conexant.c
@@ -943,6 +943,7 @@ static const struct snd_pci_quirk cxt5066_fixups[] = {
SND_PCI_QUIRK(0x17aa, 0x21da, "Lenovo X220", CXT_PINCFG_LENOVO_TP410),
SND_PCI_QUIRK(0x17aa, 0x21db, "Lenovo X220-tablet",
CXT_PINCFG_LENOVO_TP410),
SND_PCI_QUIRK(0x17aa, 0x38af, "Lenovo IdeaPad Z560",
CXT_FIXUP_MUTE_LED_EAPD),
+ SND_PCI_QUIRK(0x17aa, 0x3905, "Lenovo G50-30", CXT_FIXUP_STEREO_DMIC),
SND_PCI_QUIRK(0x17aa, 0x390b, "Lenovo G50-80", CXT_FIXUP_STEREO_DMIC),
SND_PCI_QUIRK(0x17aa, 0x3975, "Lenovo U300s", CXT_FIXUP_STEREO_DMIC),
SND_PCI_QUIRK(0x17aa, 0x3977, "Lenovo IdeaPad U310",
CXT_FIXUP_STEREO_DMIC),
--
2.19.1
Re: [PATCH] scripts: spdxcheck: resovle decode warning for python3
Hi Shubham, On 10/01/2018 04:38 AM, Shubham Singh wrote: > From: shubhsherl > > Resolve a warning issue by spdxcheck in Python3 > FAIL: 'str' object has no attribute 'decode' > > Signed-off-by: Shubham Singh > --- > scripts/spdxcheck.py | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/scripts/spdxcheck.py b/scripts/spdxcheck.py > index 839e190bbd7a..6eea4073f28a 100755 > --- a/scripts/spdxcheck.py > +++ b/scripts/spdxcheck.py > @@ -168,7 +168,8 @@ class id_parser(object): > self.curline = 0 > try: > for line in fd: > -line = line.decode(locale.getpreferredencoding(False), > errors='ignore') > +if type(line) != str: > +line = line.decode(locale.getpreferredencoding(False), > errors='ignore') > self.curline += 1 > if self.curline > maxlines: > break > This looks to be a fix for the same issue addressed by "[PATCH] scripts/spdxcheck.py: improve Python 3 compat"[0]. Can you reproduce the warning with that patch? [0] https://marc.info/?l=linux-kernel=153713239404809
Re: [PATCH] scripts: spdxcheck: resovle decode warning for python3
Hi Shubham, On 10/01/2018 04:38 AM, Shubham Singh wrote: > From: shubhsherl > > Resolve a warning issue by spdxcheck in Python3 > FAIL: 'str' object has no attribute 'decode' > > Signed-off-by: Shubham Singh > --- > scripts/spdxcheck.py | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/scripts/spdxcheck.py b/scripts/spdxcheck.py > index 839e190bbd7a..6eea4073f28a 100755 > --- a/scripts/spdxcheck.py > +++ b/scripts/spdxcheck.py > @@ -168,7 +168,8 @@ class id_parser(object): > self.curline = 0 > try: > for line in fd: > -line = line.decode(locale.getpreferredencoding(False), > errors='ignore') > +if type(line) != str: > +line = line.decode(locale.getpreferredencoding(False), > errors='ignore') > self.curline += 1 > if self.curline > maxlines: > break > This looks to be a fix for the same issue addressed by "[PATCH] scripts/spdxcheck.py: improve Python 3 compat"[0]. Can you reproduce the warning with that patch? [0] https://marc.info/?l=linux-kernel=153713239404809
Re: [PATCH] scripts/spdxcheck.py: improve Python 3 compat
On 09/16/2018 05:12 PM, Thomas Weißschuh wrote: > When reading lines from a text-mode fd strings are returned. > These can not be decoded again into strings, breaking the logic in > parser. > Just make sure all files are opened in binary mode on Python 3, so the > current logic keeps working. > > This remains compatible with Python 2 and should have no functional > change. > > Signed-off-by: Thomas Weißschuh > --- > scripts/spdxcheck.py | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/scripts/spdxcheck.py b/scripts/spdxcheck.py > index 839e190bbd7a..8f472f995d70 100755 > --- a/scripts/spdxcheck.py > +++ b/scripts/spdxcheck.py > @@ -250,12 +250,15 @@ if __name__ == '__main__': > > try: > if len(args.path) and args.path[0] == '-': > -parser.parse_lines(sys.stdin, args.maxlines, '-') > +parser.parse_lines( > +# always get the binary fd > +getattr(sys.stdin, 'buffer', sys.stdin), > +args.maxlines, '-') I think a slightly more "Pythonic" way to do this would be: try: parser.parse_lines(sys.stdin.buffer, args.maxlines, '-') except AttributeError: # Python 2 doesn't have a binary buffer interface on stdin parser.parse_lines(sys.stdin, args.maxlines, '-') but they're equivalent so it's completely personal preference. > else: > if args.path: > for p in args.path: > if os.path.isfile(p): > -parser.parse_lines(open(p), args.maxlines, p) > +parser.parse_lines(open(p, 'rb'), args.maxlines, p) > elif os.path.isdir(p): > scan_git_subtree(repo.head.reference.commit.tree, p) > else: > For what it's worth: Reviewed-by: Jeremy Cline
Re: [PATCH] scripts/spdxcheck.py: improve Python 3 compat
On 09/16/2018 05:12 PM, Thomas Weißschuh wrote: > When reading lines from a text-mode fd strings are returned. > These can not be decoded again into strings, breaking the logic in > parser. > Just make sure all files are opened in binary mode on Python 3, so the > current logic keeps working. > > This remains compatible with Python 2 and should have no functional > change. > > Signed-off-by: Thomas Weißschuh > --- > scripts/spdxcheck.py | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/scripts/spdxcheck.py b/scripts/spdxcheck.py > index 839e190bbd7a..8f472f995d70 100755 > --- a/scripts/spdxcheck.py > +++ b/scripts/spdxcheck.py > @@ -250,12 +250,15 @@ if __name__ == '__main__': > > try: > if len(args.path) and args.path[0] == '-': > -parser.parse_lines(sys.stdin, args.maxlines, '-') > +parser.parse_lines( > +# always get the binary fd > +getattr(sys.stdin, 'buffer', sys.stdin), > +args.maxlines, '-') I think a slightly more "Pythonic" way to do this would be: try: parser.parse_lines(sys.stdin.buffer, args.maxlines, '-') except AttributeError: # Python 2 doesn't have a binary buffer interface on stdin parser.parse_lines(sys.stdin, args.maxlines, '-') but they're equivalent so it's completely personal preference. > else: > if args.path: > for p in args.path: > if os.path.isfile(p): > -parser.parse_lines(open(p), args.maxlines, p) > +parser.parse_lines(open(p, 'rb'), args.maxlines, p) > elif os.path.isdir(p): > scan_git_subtree(repo.head.reference.commit.tree, p) > else: > For what it's worth: Reviewed-by: Jeremy Cline
Re: [PATCH] cpufreq: governor: Protect cpufreq governor_data
Hi folks,
On 08/14/2018 08:01 PM, Henry Willard wrote:
> If cppc_cpufreq.ko is deleted at the same time that tuned-adm is,
> changing profiles, there is a small chance that a race can occur
> between cpufreq_dbs_governor_exit() and cpufreq_dbs_governor_limits()
> resulting in a system failure when the latter tries to use
> policy->governor_data that has been freed by the former.
>
> This patch uses gov_dbs_data_mutex to synchronize access.
>
> Signed-off-by: Henry Willard
> ---
> drivers/cpufreq/cpufreq_governor.c | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/cpufreq_governor.c
> b/drivers/cpufreq/cpufreq_governor.c
> index 1d50e97d49f1..43416ee55849 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -555,12 +555,20 @@ void cpufreq_dbs_governor_stop(struct cpufreq_policy
> *policy)
>
> void cpufreq_dbs_governor_limits(struct cpufreq_policy *policy)
> {
> - struct policy_dbs_info *policy_dbs = policy->governor_data;
> + struct policy_dbs_info *policy_dbs;
> +
> + /* Protect gov->gdbs_data against cpufreq_dbs_governor_exit */
> + mutex_lock(_dbs_data_mutex);
> + policy_dbs = policy->governor_data;
> + if (!policy_dbs)
> + goto out;
>
> mutex_lock(_dbs->update_mutex);
> cpufreq_policy_apply_limits(policy);
> gov_update_sample_delay(policy_dbs, 0);
>
> mutex_unlock(_dbs->update_mutex);
> +out:
> + mutex_unlock(_dbs_data_mutex);
> }
> EXPORT_SYMBOL_GPL(cpufreq_dbs_governor_limits);
>
Fedora has gotten a report[0] of possible circular locking which looks
like might have been introduced by this patch.
[0] https://bugzilla.redhat.com/show_bug.cgi?id=1627950
User-provided log (attached to the bug report) is:
==
WARNING: possible circular locking dependency detected
4.19.0-0.rc2.git3.1.fc30.x86_64 #1 Not tainted
--
kworker/0:3/101 is trying to acquire lock:
3b2babfd ((work_completion)()){+.+.}, at:
__flush_work+0x28c/0x320
but task is already holding lock:
84d461de (_dbs->update_mutex){+.+.}, at:
dbs_work_handler+0x2b/0x70
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #5 (_dbs->update_mutex){+.+.}:
cpufreq_dbs_governor_limits+0x31/0x80
cpufreq_start_governor+0x65/0xa0
cpufreq_set_policy+0x2b7/0x330
cpufreq_init_policy+0x76/0xe0
cpufreq_online+0x535/0x6b0
cpufreq_add_dev+0x6b/0x80
subsys_interface_register+0xf3/0x150
cpufreq_register_driver+0x11e/0x230
powernowk8_init+0x134/0x18f [powernow_k8]
do_one_initcall+0x5d/0x2be
do_init_module+0x5a/0x210
load_module+0x2134/0x2400
__do_sys_init_module+0x150/0x1a0
do_syscall_64+0x60/0x1f0
entry_SYSCALL_64_after_hwframe+0x49/0xbe
-> #4 (gov_dbs_data_mutex){+.+.}:
cpufreq_dbs_governor_init+0x117/0x240
cpufreq_init_governor+0x62/0xe0
cpufreq_set_policy+0x1ca/0x330
cpufreq_init_policy+0x76/0xe0
cpufreq_online+0x535/0x6b0
cpufreq_add_dev+0x6b/0x80
subsys_interface_register+0xf3/0x150
cpufreq_register_driver+0x11e/0x230
powernowk8_init+0x134/0x18f [powernow_k8]
do_one_initcall+0x5d/0x2be
do_init_module+0x5a/0x210
load_module+0x2134/0x2400
__do_sys_init_module+0x150/0x1a0
do_syscall_64+0x60/0x1f0
entry_SYSCALL_64_after_hwframe+0x49/0xbe
-> #3 (>rwsem){+.+.}:
cpufreq_policy_free+0xba/0x140
cpufreq_online+0xec/0x6b0
cpufreq_add_dev+0x6b/0x80
subsys_interface_register+0xf3/0x150
cpufreq_register_driver+0x11e/0x230
0xc05fc25a
do_one_initcall+0x5d/0x2be
do_init_module+0x5a/0x210
load_module+0x2134/0x2400
__do_sys_init_module+0x150/0x1a0
do_syscall_64+0x60/0x1f0
entry_SYSCALL_64_after_hwframe+0x49/0xbe
-> #2 (subsys mutex#7){+.+.}:
subsys_interface_register+0x73/0x150
cpufreq_register_driver+0x11e/0x230
0xc05fc25a
do_one_initcall+0x5d/0x2be
do_init_module+0x5a/0x210
load_module+0x2134/0x2400
__do_sys_init_module+0x150/0x1a0
do_syscall_64+0x60/0x1f0
entry_SYSCALL_64_after_hwframe+0x49/0xbe
-> #1 (cpu_hotplug_lock.rw_sem){.+.+}:
apply_workqueue_attrs+0x12/0x50
__alloc_workqueue_key+0x303/0x480
scsi_host_alloc+0x3be/0x450
ata_scsi_add_hosts+0xc9/0x150
ata_host_register+0x120/0x1d0
ata_host_activate+0xc1/0x140
svia_init_one+0x6c7/0x7f6 [sata_via]
local_pci_probe+0x41/0x90
work_for_cpu_fn+0x16/0x20
process_one_work+0x27d/0x5c0
worker_thread+0x3c/0x390
kthread+0x120/0x140
ret_from_fork+0x3a/0x50
->
Re: [PATCH] cpufreq: governor: Protect cpufreq governor_data
Hi folks,
On 08/14/2018 08:01 PM, Henry Willard wrote:
> If cppc_cpufreq.ko is deleted at the same time that tuned-adm is,
> changing profiles, there is a small chance that a race can occur
> between cpufreq_dbs_governor_exit() and cpufreq_dbs_governor_limits()
> resulting in a system failure when the latter tries to use
> policy->governor_data that has been freed by the former.
>
> This patch uses gov_dbs_data_mutex to synchronize access.
>
> Signed-off-by: Henry Willard
> ---
> drivers/cpufreq/cpufreq_governor.c | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/cpufreq_governor.c
> b/drivers/cpufreq/cpufreq_governor.c
> index 1d50e97d49f1..43416ee55849 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -555,12 +555,20 @@ void cpufreq_dbs_governor_stop(struct cpufreq_policy
> *policy)
>
> void cpufreq_dbs_governor_limits(struct cpufreq_policy *policy)
> {
> - struct policy_dbs_info *policy_dbs = policy->governor_data;
> + struct policy_dbs_info *policy_dbs;
> +
> + /* Protect gov->gdbs_data against cpufreq_dbs_governor_exit */
> + mutex_lock(_dbs_data_mutex);
> + policy_dbs = policy->governor_data;
> + if (!policy_dbs)
> + goto out;
>
> mutex_lock(_dbs->update_mutex);
> cpufreq_policy_apply_limits(policy);
> gov_update_sample_delay(policy_dbs, 0);
>
> mutex_unlock(_dbs->update_mutex);
> +out:
> + mutex_unlock(_dbs_data_mutex);
> }
> EXPORT_SYMBOL_GPL(cpufreq_dbs_governor_limits);
>
Fedora has gotten a report[0] of possible circular locking which looks
like might have been introduced by this patch.
[0] https://bugzilla.redhat.com/show_bug.cgi?id=1627950
User-provided log (attached to the bug report) is:
==
WARNING: possible circular locking dependency detected
4.19.0-0.rc2.git3.1.fc30.x86_64 #1 Not tainted
--
kworker/0:3/101 is trying to acquire lock:
3b2babfd ((work_completion)()){+.+.}, at:
__flush_work+0x28c/0x320
but task is already holding lock:
84d461de (_dbs->update_mutex){+.+.}, at:
dbs_work_handler+0x2b/0x70
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #5 (_dbs->update_mutex){+.+.}:
cpufreq_dbs_governor_limits+0x31/0x80
cpufreq_start_governor+0x65/0xa0
cpufreq_set_policy+0x2b7/0x330
cpufreq_init_policy+0x76/0xe0
cpufreq_online+0x535/0x6b0
cpufreq_add_dev+0x6b/0x80
subsys_interface_register+0xf3/0x150
cpufreq_register_driver+0x11e/0x230
powernowk8_init+0x134/0x18f [powernow_k8]
do_one_initcall+0x5d/0x2be
do_init_module+0x5a/0x210
load_module+0x2134/0x2400
__do_sys_init_module+0x150/0x1a0
do_syscall_64+0x60/0x1f0
entry_SYSCALL_64_after_hwframe+0x49/0xbe
-> #4 (gov_dbs_data_mutex){+.+.}:
cpufreq_dbs_governor_init+0x117/0x240
cpufreq_init_governor+0x62/0xe0
cpufreq_set_policy+0x1ca/0x330
cpufreq_init_policy+0x76/0xe0
cpufreq_online+0x535/0x6b0
cpufreq_add_dev+0x6b/0x80
subsys_interface_register+0xf3/0x150
cpufreq_register_driver+0x11e/0x230
powernowk8_init+0x134/0x18f [powernow_k8]
do_one_initcall+0x5d/0x2be
do_init_module+0x5a/0x210
load_module+0x2134/0x2400
__do_sys_init_module+0x150/0x1a0
do_syscall_64+0x60/0x1f0
entry_SYSCALL_64_after_hwframe+0x49/0xbe
-> #3 (>rwsem){+.+.}:
cpufreq_policy_free+0xba/0x140
cpufreq_online+0xec/0x6b0
cpufreq_add_dev+0x6b/0x80
subsys_interface_register+0xf3/0x150
cpufreq_register_driver+0x11e/0x230
0xc05fc25a
do_one_initcall+0x5d/0x2be
do_init_module+0x5a/0x210
load_module+0x2134/0x2400
__do_sys_init_module+0x150/0x1a0
do_syscall_64+0x60/0x1f0
entry_SYSCALL_64_after_hwframe+0x49/0xbe
-> #2 (subsys mutex#7){+.+.}:
subsys_interface_register+0x73/0x150
cpufreq_register_driver+0x11e/0x230
0xc05fc25a
do_one_initcall+0x5d/0x2be
do_init_module+0x5a/0x210
load_module+0x2134/0x2400
__do_sys_init_module+0x150/0x1a0
do_syscall_64+0x60/0x1f0
entry_SYSCALL_64_after_hwframe+0x49/0xbe
-> #1 (cpu_hotplug_lock.rw_sem){.+.+}:
apply_workqueue_attrs+0x12/0x50
__alloc_workqueue_key+0x303/0x480
scsi_host_alloc+0x3be/0x450
ata_scsi_add_hosts+0xc9/0x150
ata_host_register+0x120/0x1d0
ata_host_activate+0xc1/0x140
svia_init_one+0x6c7/0x7f6 [sata_via]
local_pci_probe+0x41/0x90
work_for_cpu_fn+0x16/0x20
process_one_work+0x27d/0x5c0
worker_thread+0x3c/0x390
kthread+0x120/0x140
ret_from_fork+0x3a/0x50
->
[PATCH 0/2] fs/quota: Fix potential spectre v1 gadgets
Hi folks, This series unifies XQM_MAXQUOTAS with MAXQUOTAS, which were both being used to perform bounds checks on arrays, and then sanitizes 'type' so it can't be used in speculative out-of-bounds array access. Jeremy Cline (2): fs/quota: Replace XQM_MAXQUOTAS usage with MAXQUOTAS fs/quota: Fix spectre gadget in do_quotactl fs/quota/quota.c | 14 +++--- include/linux/quota.h | 8 +--- include/uapi/linux/dqblk_xfs.h | 5 - 3 files changed, 8 insertions(+), 19 deletions(-) -- 2.17.1
[PATCH 2/2] fs/quota: Fix spectre gadget in do_quotactl
'type' is user-controlled, so sanitize it after the bounds check to avoid using it in speculative execution. This covers the following potential gadgets detected with the help of smatch: * fs/ext4/super.c:5741 ext4_quota_read() warn: potential spectre issue 'sb_dqopt(sb)->files' [r] * fs/ext4/super.c:5778 ext4_quota_write() warn: potential spectre issue 'sb_dqopt(sb)->files' [r] * fs/f2fs/super.c:1552 f2fs_quota_read() warn: potential spectre issue 'sb_dqopt(sb)->files' [r] * fs/f2fs/super.c:1608 f2fs_quota_write() warn: potential spectre issue 'sb_dqopt(sb)->files' [r] * fs/quota/dquot.c:412 mark_info_dirty() warn: potential spectre issue 'sb_dqopt(sb)->info' [w] * fs/quota/dquot.c:933 dqinit_needed() warn: potential spectre issue 'dquots' [r] * fs/quota/dquot.c:2112 dquot_commit_info() warn: potential spectre issue 'dqopt->ops' [r] * fs/quota/dquot.c:2362 vfs_load_quota_inode() warn: potential spectre issue 'dqopt->files' [w] (local cap) * fs/quota/dquot.c:2369 vfs_load_quota_inode() warn: potential spectre issue 'dqopt->ops' [w] (local cap) * fs/quota/dquot.c:2370 vfs_load_quota_inode() warn: potential spectre issue 'dqopt->info' [w] (local cap) * fs/quota/quota.c:110 quota_getfmt() warn: potential spectre issue 'sb_dqopt(sb)->info' [r] * fs/quota/quota_v2.c:84 v2_check_quota_file() warn: potential spectre issue 'quota_magics' [w] * fs/quota/quota_v2.c:85 v2_check_quota_file() warn: potential spectre issue 'quota_versions' [w] * fs/quota/quota_v2.c:96 v2_read_file_info() warn: potential spectre issue 'dqopt->info' [r] * fs/quota/quota_v2.c:172 v2_write_file_info() warn: potential spectre issue 'dqopt->info' [r] Additionally, a quick inspection indicates there are array accesses with 'type' in quota_on() and quota_off() functions which are also addressed by this. Cc: Josh Poimboeuf Cc: sta...@vger.kernel.org Signed-off-by: Jeremy Cline --- This patch isn't going to cleanly apply to stable without the "fs/quota: Replace XQM_MAXQUOTAS usage with MAXQUOTAS" patch, but I'm not sure that patch is really stable material and XQM_MAXQUOTAS has been 3 since pre-v4.4 so the end result will be the same even if that patch isn't backported. fs/quota/quota.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fs/quota/quota.c b/fs/quota/quota.c index d403392d8a0f..f0cbf58ad4da 100644 --- a/fs/quota/quota.c +++ b/fs/quota/quota.c @@ -18,6 +18,7 @@ #include #include #include +#include static int check_quotactl_permission(struct super_block *sb, int type, int cmd, qid_t id) @@ -701,6 +702,7 @@ static int do_quotactl(struct super_block *sb, int type, int cmd, qid_t id, if (type >= MAXQUOTAS) return -EINVAL; + type = array_index_nospec(type, MAXQUOTAS); /* * Quota not supported on this fs? Check this before s_quota_types * since they needn't be set if quota is not supported at all. -- 2.17.1
[PATCH 0/2] fs/quota: Fix potential spectre v1 gadgets
Hi folks, This series unifies XQM_MAXQUOTAS with MAXQUOTAS, which were both being used to perform bounds checks on arrays, and then sanitizes 'type' so it can't be used in speculative out-of-bounds array access. Jeremy Cline (2): fs/quota: Replace XQM_MAXQUOTAS usage with MAXQUOTAS fs/quota: Fix spectre gadget in do_quotactl fs/quota/quota.c | 14 +++--- include/linux/quota.h | 8 +--- include/uapi/linux/dqblk_xfs.h | 5 - 3 files changed, 8 insertions(+), 19 deletions(-) -- 2.17.1
[PATCH 2/2] fs/quota: Fix spectre gadget in do_quotactl
'type' is user-controlled, so sanitize it after the bounds check to avoid using it in speculative execution. This covers the following potential gadgets detected with the help of smatch: * fs/ext4/super.c:5741 ext4_quota_read() warn: potential spectre issue 'sb_dqopt(sb)->files' [r] * fs/ext4/super.c:5778 ext4_quota_write() warn: potential spectre issue 'sb_dqopt(sb)->files' [r] * fs/f2fs/super.c:1552 f2fs_quota_read() warn: potential spectre issue 'sb_dqopt(sb)->files' [r] * fs/f2fs/super.c:1608 f2fs_quota_write() warn: potential spectre issue 'sb_dqopt(sb)->files' [r] * fs/quota/dquot.c:412 mark_info_dirty() warn: potential spectre issue 'sb_dqopt(sb)->info' [w] * fs/quota/dquot.c:933 dqinit_needed() warn: potential spectre issue 'dquots' [r] * fs/quota/dquot.c:2112 dquot_commit_info() warn: potential spectre issue 'dqopt->ops' [r] * fs/quota/dquot.c:2362 vfs_load_quota_inode() warn: potential spectre issue 'dqopt->files' [w] (local cap) * fs/quota/dquot.c:2369 vfs_load_quota_inode() warn: potential spectre issue 'dqopt->ops' [w] (local cap) * fs/quota/dquot.c:2370 vfs_load_quota_inode() warn: potential spectre issue 'dqopt->info' [w] (local cap) * fs/quota/quota.c:110 quota_getfmt() warn: potential spectre issue 'sb_dqopt(sb)->info' [r] * fs/quota/quota_v2.c:84 v2_check_quota_file() warn: potential spectre issue 'quota_magics' [w] * fs/quota/quota_v2.c:85 v2_check_quota_file() warn: potential spectre issue 'quota_versions' [w] * fs/quota/quota_v2.c:96 v2_read_file_info() warn: potential spectre issue 'dqopt->info' [r] * fs/quota/quota_v2.c:172 v2_write_file_info() warn: potential spectre issue 'dqopt->info' [r] Additionally, a quick inspection indicates there are array accesses with 'type' in quota_on() and quota_off() functions which are also addressed by this. Cc: Josh Poimboeuf Cc: sta...@vger.kernel.org Signed-off-by: Jeremy Cline --- This patch isn't going to cleanly apply to stable without the "fs/quota: Replace XQM_MAXQUOTAS usage with MAXQUOTAS" patch, but I'm not sure that patch is really stable material and XQM_MAXQUOTAS has been 3 since pre-v4.4 so the end result will be the same even if that patch isn't backported. fs/quota/quota.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fs/quota/quota.c b/fs/quota/quota.c index d403392d8a0f..f0cbf58ad4da 100644 --- a/fs/quota/quota.c +++ b/fs/quota/quota.c @@ -18,6 +18,7 @@ #include #include #include +#include static int check_quotactl_permission(struct super_block *sb, int type, int cmd, qid_t id) @@ -701,6 +702,7 @@ static int do_quotactl(struct super_block *sb, int type, int cmd, qid_t id, if (type >= MAXQUOTAS) return -EINVAL; + type = array_index_nospec(type, MAXQUOTAS); /* * Quota not supported on this fs? Check this before s_quota_types * since they needn't be set if quota is not supported at all. -- 2.17.1
[PATCH 1/2] fs/quota: Replace XQM_MAXQUOTAS usage with MAXQUOTAS
XQM_MAXQUOTAS and MAXQUOTAS are, it appears, equivalent. Replace all
usage of XQM_MAXQUOTAS and remove it along with the unused XQM_*QUOTA
definitions.
Signed-off-by: Jeremy Cline
---
fs/quota/quota.c | 12 +---
include/linux/quota.h | 8 +---
include/uapi/linux/dqblk_xfs.h | 5 -
3 files changed, 6 insertions(+), 19 deletions(-)
diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index 860bfbe7a07a..d403392d8a0f 100644
--- a/fs/quota/quota.c
+++ b/fs/quota/quota.c
@@ -120,8 +120,6 @@ static int quota_getinfo(struct super_block *sb, int type,
void __user *addr)
struct if_dqinfo uinfo;
int ret;
- /* This checks whether qc_state has enough entries... */
- BUILD_BUG_ON(MAXQUOTAS > XQM_MAXQUOTAS);
if (!sb->s_qcop->get_state)
return -ENOSYS;
ret = sb->s_qcop->get_state(sb, );
@@ -354,10 +352,10 @@ static int quota_getstate(struct super_block *sb, struct
fs_quota_stat *fqs)
* GETXSTATE quotactl has space for just one set of time limits so
* report them for the first enabled quota type
*/
- for (type = 0; type < XQM_MAXQUOTAS; type++)
+ for (type = 0; type < MAXQUOTAS; type++)
if (state.s_state[type].flags & QCI_ACCT_ENABLED)
break;
- BUG_ON(type == XQM_MAXQUOTAS);
+ BUG_ON(type == MAXQUOTAS);
fqs->qs_btimelimit = state.s_state[type].spc_timelimit;
fqs->qs_itimelimit = state.s_state[type].ino_timelimit;
fqs->qs_rtbtimelimit = state.s_state[type].rt_spc_timelimit;
@@ -427,10 +425,10 @@ static int quota_getstatev(struct super_block *sb, struct
fs_quota_statv *fqs)
* GETXSTATV quotactl has space for just one set of time limits so
* report them for the first enabled quota type
*/
- for (type = 0; type < XQM_MAXQUOTAS; type++)
+ for (type = 0; type < MAXQUOTAS; type++)
if (state.s_state[type].flags & QCI_ACCT_ENABLED)
break;
- BUG_ON(type == XQM_MAXQUOTAS);
+ BUG_ON(type == MAXQUOTAS);
fqs->qs_btimelimit = state.s_state[type].spc_timelimit;
fqs->qs_itimelimit = state.s_state[type].ino_timelimit;
fqs->qs_rtbtimelimit = state.s_state[type].rt_spc_timelimit;
@@ -701,7 +699,7 @@ static int do_quotactl(struct super_block *sb, int type,
int cmd, qid_t id,
{
int ret;
- if (type >= (XQM_COMMAND(cmd) ? XQM_MAXQUOTAS : MAXQUOTAS))
+ if (type >= MAXQUOTAS)
return -EINVAL;
/*
* Quota not supported on this fs? Check this before s_quota_types
diff --git a/include/linux/quota.h b/include/linux/quota.h
index ca9772c8e48b..f32dd270b8e3 100644
--- a/include/linux/quota.h
+++ b/include/linux/quota.h
@@ -408,13 +408,7 @@ struct qc_type_state {
struct qc_state {
unsigned int s_incoredqs; /* Number of dquots in core */
- /*
-* Per quota type information. The array should really have
-* max(MAXQUOTAS, XQM_MAXQUOTAS) entries. BUILD_BUG_ON in
-* quota_getinfo() makes sure XQM_MAXQUOTAS is large enough. Once VFS
-* supports project quotas, this can be changed to MAXQUOTAS
-*/
- struct qc_type_state s_state[XQM_MAXQUOTAS];
+ struct qc_type_state s_state[MAXQUOTAS]; /* Per quota type information
*/
};
/* Structure for communicating via ->set_info */
diff --git a/include/uapi/linux/dqblk_xfs.h b/include/uapi/linux/dqblk_xfs.h
index 03d890b80ebc..a5846cde8272 100644
--- a/include/uapi/linux/dqblk_xfs.h
+++ b/include/uapi/linux/dqblk_xfs.h
@@ -27,11 +27,6 @@
#define XQM_CMD(x) (('X'<<8)+(x)) /* note: forms first QCMD argument */
#define XQM_COMMAND(x) (((x) & (0xff<<8)) == ('X'<<8)) /* test if for XFS */
-#define XQM_USRQUOTA 0 /* system call user quota type */
-#define XQM_GRPQUOTA 1 /* system call group quota type */
-#define XQM_PRJQUOTA 2 /* system call project quota type */
-#define XQM_MAXQUOTAS 3
-
#define Q_XQUOTAON XQM_CMD(1) /* enable accounting/enforcement */
#define Q_XQUOTAOFFXQM_CMD(2) /* disable accounting/enforcement */
#define Q_XGETQUOTAXQM_CMD(3) /* get disk limits and usage */
--
2.17.1
[PATCH 1/2] fs/quota: Replace XQM_MAXQUOTAS usage with MAXQUOTAS
XQM_MAXQUOTAS and MAXQUOTAS are, it appears, equivalent. Replace all
usage of XQM_MAXQUOTAS and remove it along with the unused XQM_*QUOTA
definitions.
Signed-off-by: Jeremy Cline
---
fs/quota/quota.c | 12 +---
include/linux/quota.h | 8 +---
include/uapi/linux/dqblk_xfs.h | 5 -
3 files changed, 6 insertions(+), 19 deletions(-)
diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index 860bfbe7a07a..d403392d8a0f 100644
--- a/fs/quota/quota.c
+++ b/fs/quota/quota.c
@@ -120,8 +120,6 @@ static int quota_getinfo(struct super_block *sb, int type,
void __user *addr)
struct if_dqinfo uinfo;
int ret;
- /* This checks whether qc_state has enough entries... */
- BUILD_BUG_ON(MAXQUOTAS > XQM_MAXQUOTAS);
if (!sb->s_qcop->get_state)
return -ENOSYS;
ret = sb->s_qcop->get_state(sb, );
@@ -354,10 +352,10 @@ static int quota_getstate(struct super_block *sb, struct
fs_quota_stat *fqs)
* GETXSTATE quotactl has space for just one set of time limits so
* report them for the first enabled quota type
*/
- for (type = 0; type < XQM_MAXQUOTAS; type++)
+ for (type = 0; type < MAXQUOTAS; type++)
if (state.s_state[type].flags & QCI_ACCT_ENABLED)
break;
- BUG_ON(type == XQM_MAXQUOTAS);
+ BUG_ON(type == MAXQUOTAS);
fqs->qs_btimelimit = state.s_state[type].spc_timelimit;
fqs->qs_itimelimit = state.s_state[type].ino_timelimit;
fqs->qs_rtbtimelimit = state.s_state[type].rt_spc_timelimit;
@@ -427,10 +425,10 @@ static int quota_getstatev(struct super_block *sb, struct
fs_quota_statv *fqs)
* GETXSTATV quotactl has space for just one set of time limits so
* report them for the first enabled quota type
*/
- for (type = 0; type < XQM_MAXQUOTAS; type++)
+ for (type = 0; type < MAXQUOTAS; type++)
if (state.s_state[type].flags & QCI_ACCT_ENABLED)
break;
- BUG_ON(type == XQM_MAXQUOTAS);
+ BUG_ON(type == MAXQUOTAS);
fqs->qs_btimelimit = state.s_state[type].spc_timelimit;
fqs->qs_itimelimit = state.s_state[type].ino_timelimit;
fqs->qs_rtbtimelimit = state.s_state[type].rt_spc_timelimit;
@@ -701,7 +699,7 @@ static int do_quotactl(struct super_block *sb, int type,
int cmd, qid_t id,
{
int ret;
- if (type >= (XQM_COMMAND(cmd) ? XQM_MAXQUOTAS : MAXQUOTAS))
+ if (type >= MAXQUOTAS)
return -EINVAL;
/*
* Quota not supported on this fs? Check this before s_quota_types
diff --git a/include/linux/quota.h b/include/linux/quota.h
index ca9772c8e48b..f32dd270b8e3 100644
--- a/include/linux/quota.h
+++ b/include/linux/quota.h
@@ -408,13 +408,7 @@ struct qc_type_state {
struct qc_state {
unsigned int s_incoredqs; /* Number of dquots in core */
- /*
-* Per quota type information. The array should really have
-* max(MAXQUOTAS, XQM_MAXQUOTAS) entries. BUILD_BUG_ON in
-* quota_getinfo() makes sure XQM_MAXQUOTAS is large enough. Once VFS
-* supports project quotas, this can be changed to MAXQUOTAS
-*/
- struct qc_type_state s_state[XQM_MAXQUOTAS];
+ struct qc_type_state s_state[MAXQUOTAS]; /* Per quota type information
*/
};
/* Structure for communicating via ->set_info */
diff --git a/include/uapi/linux/dqblk_xfs.h b/include/uapi/linux/dqblk_xfs.h
index 03d890b80ebc..a5846cde8272 100644
--- a/include/uapi/linux/dqblk_xfs.h
+++ b/include/uapi/linux/dqblk_xfs.h
@@ -27,11 +27,6 @@
#define XQM_CMD(x) (('X'<<8)+(x)) /* note: forms first QCMD argument */
#define XQM_COMMAND(x) (((x) & (0xff<<8)) == ('X'<<8)) /* test if for XFS */
-#define XQM_USRQUOTA 0 /* system call user quota type */
-#define XQM_GRPQUOTA 1 /* system call group quota type */
-#define XQM_PRJQUOTA 2 /* system call project quota type */
-#define XQM_MAXQUOTAS 3
-
#define Q_XQUOTAON XQM_CMD(1) /* enable accounting/enforcement */
#define Q_XQUOTAOFFXQM_CMD(2) /* disable accounting/enforcement */
#define Q_XGETQUOTAXQM_CMD(3) /* get disk limits and usage */
--
2.17.1
Re: [PATCH v2] ext4: mballoc: Fix spectre gadget in ext4_mb_regular_allocator
Hi Ted, On 07/30/2018 02:36 PM, Theodore Y. Ts'o wrote: > Hey Jeremy, > > I think you are also going to be changing the 1/3 patch from the > original patch series that this was part of. That's correct, right? > > It would be easier for me if you could simply make all of the > revisions you plan to make for the patch series, and then upload a > full v2 of the entire patch series. > > Right now I've mentally marked the entire patch series as "waiting > forh v2". So when you send a V2 version of individual patch out of > that series, it leaves things unclear when/if you plan to update any > of other patches in the series. I dropped patch 1/3 and 2/3 from the original series because they can both be covered by some sanitation in fs/quota/quota.c, so the this is only patch from the v1 series that should be applied. Sorry for not being more clear! Cheers, Jeremy
Re: [PATCH v2] ext4: mballoc: Fix spectre gadget in ext4_mb_regular_allocator
Hi Ted, On 07/30/2018 02:36 PM, Theodore Y. Ts'o wrote: > Hey Jeremy, > > I think you are also going to be changing the 1/3 patch from the > original patch series that this was part of. That's correct, right? > > It would be easier for me if you could simply make all of the > revisions you plan to make for the patch series, and then upload a > full v2 of the entire patch series. > > Right now I've mentally marked the entire patch series as "waiting > forh v2". So when you send a V2 version of individual patch out of > that series, it leaves things unclear when/if you plan to update any > of other patches in the series. I dropped patch 1/3 and 2/3 from the original series because they can both be covered by some sanitation in fs/quota/quota.c, so the this is only patch from the v1 series that should be applied. Sorry for not being more clear! Cheers, Jeremy
[PATCH v2] ext4: mballoc: Fix spectre gadget in ext4_mb_regular_allocator
'ac->ac_g_ex.fe_len' is a user-controlled value which is used in the derivation of 'ac->ac_2order'. 'ac->ac_2order', in turn, is used to index arrays which makes it a potential spectre gadget. Fix this by sanitizing the value assigned to 'ac->ac2_order'. This covers the following accesses found with the help of smatch: * fs/ext4/mballoc.c:1896 ext4_mb_simple_scan_group() warn: potential spectre issue 'grp->bb_counters' [w] (local cap) * fs/ext4/mballoc.c:445 mb_find_buddy() warn: potential spectre issue 'EXT4_SB(e4b->bd_sb)->s_mb_offsets' [r] (local cap) * fs/ext4/mballoc.c:446 mb_find_buddy() warn: potential spectre issue 'EXT4_SB(e4b->bd_sb)->s_mb_maxs' [r] (local cap) Cc: Josh Poimboeuf Cc: sta...@vger.kernel.org Suggested-by: Josh Poimboeuf Signed-off-by: Jeremy Cline --- I broke this out of the "ext4: fix spectre v1 gadgets" patch set since the other patches in that series could, as Josh noted, be replaced with one fix in do_quotactl. I'll send that fix to the disk quota folks separately. Changes from v1: - Sanitize ac_2order on assignment, rather than down the call chain in ext4_mb_simple_scan_group. fs/ext4/mballoc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c index f7ab34088162..8b24d3d42cb3 100644 --- a/fs/ext4/mballoc.c +++ b/fs/ext4/mballoc.c @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -2140,7 +2141,8 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac) * This should tell if fe_len is exactly power of 2 */ if ((ac->ac_g_ex.fe_len & (~(1 << (i - 1 == 0) - ac->ac_2order = i - 1; + ac->ac_2order = array_index_nospec(i - 1, + sb->s_blocksize_bits + 2); } /* if stream allocation is enabled, use global goal */ -- 2.17.1
[PATCH v2] ext4: mballoc: Fix spectre gadget in ext4_mb_regular_allocator
'ac->ac_g_ex.fe_len' is a user-controlled value which is used in the derivation of 'ac->ac_2order'. 'ac->ac_2order', in turn, is used to index arrays which makes it a potential spectre gadget. Fix this by sanitizing the value assigned to 'ac->ac2_order'. This covers the following accesses found with the help of smatch: * fs/ext4/mballoc.c:1896 ext4_mb_simple_scan_group() warn: potential spectre issue 'grp->bb_counters' [w] (local cap) * fs/ext4/mballoc.c:445 mb_find_buddy() warn: potential spectre issue 'EXT4_SB(e4b->bd_sb)->s_mb_offsets' [r] (local cap) * fs/ext4/mballoc.c:446 mb_find_buddy() warn: potential spectre issue 'EXT4_SB(e4b->bd_sb)->s_mb_maxs' [r] (local cap) Cc: Josh Poimboeuf Cc: sta...@vger.kernel.org Suggested-by: Josh Poimboeuf Signed-off-by: Jeremy Cline --- I broke this out of the "ext4: fix spectre v1 gadgets" patch set since the other patches in that series could, as Josh noted, be replaced with one fix in do_quotactl. I'll send that fix to the disk quota folks separately. Changes from v1: - Sanitize ac_2order on assignment, rather than down the call chain in ext4_mb_simple_scan_group. fs/ext4/mballoc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c index f7ab34088162..8b24d3d42cb3 100644 --- a/fs/ext4/mballoc.c +++ b/fs/ext4/mballoc.c @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -2140,7 +2141,8 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac) * This should tell if fe_len is exactly power of 2 */ if ((ac->ac_g_ex.fe_len & (~(1 << (i - 1 == 0) - ac->ac_2order = i - 1; + ac->ac_2order = array_index_nospec(i - 1, + sb->s_blocksize_bits + 2); } /* if stream allocation is enabled, use global goal */ -- 2.17.1
[PATCH v2] scripts: Add Python 3 support to tracing/draw_functrace.py
Use the print function. This maintains Python 2 support and should have no functional change. Signed-off-by: Jeremy Cline --- Changes from v1: - Drop "from __future__ import print_function" as Python 2 is not long for this world and in this case the statement and function behave the same way. scripts/tracing/draw_functrace.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/tracing/draw_functrace.py b/scripts/tracing/draw_functrace.py index db40fa04cd51..9b6dd4f36335 100755 --- a/scripts/tracing/draw_functrace.py +++ b/scripts/tracing/draw_functrace.py @@ -123,7 +123,7 @@ def main(): tree = tree.getParent(caller) tree = tree.calls(callee, calltime) - print CallTree.ROOT + print(CallTree.ROOT) if __name__ == "__main__": main() -- 2.17.1
[PATCH v2] scripts: Add Python 3 support to tracing/draw_functrace.py
Use the print function. This maintains Python 2 support and should have no functional change. Signed-off-by: Jeremy Cline --- Changes from v1: - Drop "from __future__ import print_function" as Python 2 is not long for this world and in this case the statement and function behave the same way. scripts/tracing/draw_functrace.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/tracing/draw_functrace.py b/scripts/tracing/draw_functrace.py index db40fa04cd51..9b6dd4f36335 100755 --- a/scripts/tracing/draw_functrace.py +++ b/scripts/tracing/draw_functrace.py @@ -123,7 +123,7 @@ def main(): tree = tree.getParent(caller) tree = tree.calls(callee, calltime) - print CallTree.ROOT + print(CallTree.ROOT) if __name__ == "__main__": main() -- 2.17.1
Re: [PATCH 1/3] ext4: super: Fix spectre gadget in ext4_quota_on
On 07/27/2018 01:46 PM, Josh Poimboeuf wrote:
> On Fri, Jul 27, 2018 at 04:23:55PM +0000, Jeremy Cline wrote:
>> 'type' is a user-controlled value used to index into 's_qf_names', which
>> can be used in a Spectre v1 attack. Clamp 'type' to the size of the
>> array to avoid a speculative out-of-bounds read.
>>
>> Cc: Josh Poimboeuf
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Jeremy Cline
>> ---
>> fs/ext4/super.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 6480e763080f..c04a09b51742 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -40,6 +40,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>>
>> @@ -5559,6 +5560,7 @@ static int ext4_quota_on(struct super_block *sb, int
>> type, int format_id,
>> if (path->dentry->d_sb != sb)
>> return -EXDEV;
>> /* Journaling quota? */
>> +type = array_index_nospec(type, EXT4_MAXQUOTAS);
>> if (EXT4_SB(sb)->s_qf_names[type]) {
>> /* Quotafile not in fs root? */
>> if (path->dentry->d_parent != sb->s_root)
>
> Generally we try to put the array_index_nospec() close to the bounds
> check for which it's trying to prevent speculation past.
>
> In this case, I'd expect the EXT4_MAXQUOTAS bounds check to be in
> do_quotactl(), but it seems to be missing:
>
> if (type >= (XQM_COMMAND(cmd) ? XQM_MAXQUOTAS : MAXQUOTAS))
> return -EINVAL;
>
> Also it looks like XQM_MAXQUOTAS, MAXQUOTAS, and EXT4_MAXQUOTAS all have
> the same value (3). Maybe they can be consolidated to just use
> MAXQUOTAS everywhere? Then the nospec would be simple:
>
> if (type >= MAXQUOTAS)
> return -EINVAL;
> type = array_index_nospec(type, MAXQUOTAS);
>
> Otherwise I think we may need to disperse the array_index_nospec calls
> deeper in the callchain.
>
Makes sense, that would be much nicer. I looked into the history a bit,
EXT4_MAXQUOTAS was adjusted from 2 to 3 in v4.5 so a backport of this to
v4.4 or earlier would require a bit of adjustment, but XQM_MAXQUOTAS
looks to have been 3 for a very long time.
I'll see about consolidating them and adjusting this patch.
Thanks,
Jeremy
Re: [PATCH 1/3] ext4: super: Fix spectre gadget in ext4_quota_on
On 07/27/2018 01:46 PM, Josh Poimboeuf wrote:
> On Fri, Jul 27, 2018 at 04:23:55PM +0000, Jeremy Cline wrote:
>> 'type' is a user-controlled value used to index into 's_qf_names', which
>> can be used in a Spectre v1 attack. Clamp 'type' to the size of the
>> array to avoid a speculative out-of-bounds read.
>>
>> Cc: Josh Poimboeuf
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Jeremy Cline
>> ---
>> fs/ext4/super.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 6480e763080f..c04a09b51742 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -40,6 +40,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>>
>> @@ -5559,6 +5560,7 @@ static int ext4_quota_on(struct super_block *sb, int
>> type, int format_id,
>> if (path->dentry->d_sb != sb)
>> return -EXDEV;
>> /* Journaling quota? */
>> +type = array_index_nospec(type, EXT4_MAXQUOTAS);
>> if (EXT4_SB(sb)->s_qf_names[type]) {
>> /* Quotafile not in fs root? */
>> if (path->dentry->d_parent != sb->s_root)
>
> Generally we try to put the array_index_nospec() close to the bounds
> check for which it's trying to prevent speculation past.
>
> In this case, I'd expect the EXT4_MAXQUOTAS bounds check to be in
> do_quotactl(), but it seems to be missing:
>
> if (type >= (XQM_COMMAND(cmd) ? XQM_MAXQUOTAS : MAXQUOTAS))
> return -EINVAL;
>
> Also it looks like XQM_MAXQUOTAS, MAXQUOTAS, and EXT4_MAXQUOTAS all have
> the same value (3). Maybe they can be consolidated to just use
> MAXQUOTAS everywhere? Then the nospec would be simple:
>
> if (type >= MAXQUOTAS)
> return -EINVAL;
> type = array_index_nospec(type, MAXQUOTAS);
>
> Otherwise I think we may need to disperse the array_index_nospec calls
> deeper in the callchain.
>
Makes sense, that would be much nicer. I looked into the history a bit,
EXT4_MAXQUOTAS was adjusted from 2 to 3 in v4.5 so a backport of this to
v4.4 or earlier would require a bit of adjustment, but XQM_MAXQUOTAS
looks to have been 3 for a very long time.
I'll see about consolidating them and adjusting this patch.
Thanks,
Jeremy
[PATCH 3/3] ext4: mballoc: Fix spectre gadget in ext4_mb_simple_scan_group
'ac->ac_2order' is a user-controlled value used to index into
'grp->bb_counters' and based on the value at that index, 'ac->ac_found'
is written to. Clamp the value right after the bounds check to avoid a
speculative out-of-bounds read of 'grp->bb_counters'.
This also protects the access of the s_mb_offsets and s_mb_maxs arrays
inside mb_find_buddy().
These gadgets were discovered with the help of smatch:
* fs/ext4/mballoc.c:1896 ext4_mb_simple_scan_group() warn: potential
spectre issue 'grp->bb_counters' [w] (local cap)
* fs/ext4/mballoc.c:445 mb_find_buddy() warn: potential spectre issue
'EXT4_SB(e4b->bd_sb)->s_mb_offsets' [r] (local cap)
* fs/ext4/mballoc.c:446 mb_find_buddy() warn: potential spectre issue
'EXT4_SB(e4b->bd_sb)->s_mb_maxs' [r] (local cap)
Cc: Josh Poimboeuf
Cc: sta...@vger.kernel.org
Signed-off-by: Jeremy Cline
---
fs/ext4/mballoc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index f7ab34088162..c0866007a949 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -14,6 +14,7 @@
#include
#include
#include
+#include
#include
#include
@@ -1893,6 +1894,7 @@ void ext4_mb_simple_scan_group(struct
ext4_allocation_context *ac,
BUG_ON(ac->ac_2order <= 0);
for (i = ac->ac_2order; i <= sb->s_blocksize_bits + 1; i++) {
+ i = array_index_nospec(i, sb->s_blocksize_bits + 2);
if (grp->bb_counters[i] == 0)
continue;
--
2.17.1
[PATCH 3/3] ext4: mballoc: Fix spectre gadget in ext4_mb_simple_scan_group
'ac->ac_2order' is a user-controlled value used to index into
'grp->bb_counters' and based on the value at that index, 'ac->ac_found'
is written to. Clamp the value right after the bounds check to avoid a
speculative out-of-bounds read of 'grp->bb_counters'.
This also protects the access of the s_mb_offsets and s_mb_maxs arrays
inside mb_find_buddy().
These gadgets were discovered with the help of smatch:
* fs/ext4/mballoc.c:1896 ext4_mb_simple_scan_group() warn: potential
spectre issue 'grp->bb_counters' [w] (local cap)
* fs/ext4/mballoc.c:445 mb_find_buddy() warn: potential spectre issue
'EXT4_SB(e4b->bd_sb)->s_mb_offsets' [r] (local cap)
* fs/ext4/mballoc.c:446 mb_find_buddy() warn: potential spectre issue
'EXT4_SB(e4b->bd_sb)->s_mb_maxs' [r] (local cap)
Cc: Josh Poimboeuf
Cc: sta...@vger.kernel.org
Signed-off-by: Jeremy Cline
---
fs/ext4/mballoc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index f7ab34088162..c0866007a949 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -14,6 +14,7 @@
#include
#include
#include
+#include
#include
#include
@@ -1893,6 +1894,7 @@ void ext4_mb_simple_scan_group(struct
ext4_allocation_context *ac,
BUG_ON(ac->ac_2order <= 0);
for (i = ac->ac_2order; i <= sb->s_blocksize_bits + 1; i++) {
+ i = array_index_nospec(i, sb->s_blocksize_bits + 2);
if (grp->bb_counters[i] == 0)
continue;
--
2.17.1
[PATCH 1/3] ext4: super: Fix spectre gadget in ext4_quota_on
'type' is a user-controlled value used to index into 's_qf_names', which
can be used in a Spectre v1 attack. Clamp 'type' to the size of the
array to avoid a speculative out-of-bounds read.
Cc: Josh Poimboeuf
Cc: sta...@vger.kernel.org
Signed-off-by: Jeremy Cline
---
fs/ext4/super.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 6480e763080f..c04a09b51742 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -40,6 +40,7 @@
#include
#include
#include
+#include
#include
#include
@@ -5559,6 +5560,7 @@ static int ext4_quota_on(struct super_block *sb, int
type, int format_id,
if (path->dentry->d_sb != sb)
return -EXDEV;
/* Journaling quota? */
+ type = array_index_nospec(type, EXT4_MAXQUOTAS);
if (EXT4_SB(sb)->s_qf_names[type]) {
/* Quotafile not in fs root? */
if (path->dentry->d_parent != sb->s_root)
--
2.17.1
[PATCH 2/3] ext4: super: Fix spectre gadgets in ext4_quota_{read,write,off}
'type' is a user-controlled value used to index 'sb_dqopt(sb)->files'.
Clamp 'type' to the size of the array to avoid a speculative
out-of-bounds read.
These gadgets were found with the help of smatch:
* fs/ext4/super.c:5741 ext4_quota_read() warn: potential spectre issue
'sb_dqopt(sb)->files' [r]
* fs/ext4/super.c:5778 ext4_quota_write() warn: potential spectre issue
'sb_dqopt(sb)->files' [r]
Cc: Josh Poimboeuf
Cc: sta...@vger.kernel.org
Signed-off-by: Jeremy Cline
---
fs/ext4/super.c | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c04a09b51742..de358eba024a 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5693,10 +5693,13 @@ static int ext4_enable_quotas(struct super_block *sb)
static int ext4_quota_off(struct super_block *sb, int type)
{
- struct inode *inode = sb_dqopt(sb)->files[type];
+ struct inode *inode;
handle_t *handle;
int err;
+ type = array_index_nospec(type, MAXQUOTAS);
+ inode = sb_dqopt(sb)->files[type];
+
/* Force all delayed allocation blocks to be allocated.
* Caller already holds s_umount sem */
if (test_opt(sb, DELALLOC))
@@ -5740,13 +5743,17 @@ static int ext4_quota_off(struct super_block *sb, int
type)
static ssize_t ext4_quota_read(struct super_block *sb, int type, char *data,
size_t len, loff_t off)
{
- struct inode *inode = sb_dqopt(sb)->files[type];
+ struct inode *inode;
ext4_lblk_t blk = off >> EXT4_BLOCK_SIZE_BITS(sb);
int offset = off & (sb->s_blocksize - 1);
int tocopy;
size_t toread;
struct buffer_head *bh;
- loff_t i_size = i_size_read(inode);
+ loff_t i_size;
+
+ type = array_index_nospec(type, MAXQUOTAS);
+ inode = sb_dqopt(sb)->files[type];
+ i_size = i_size_read(inode);
if (off > i_size)
return 0;
@@ -5777,13 +5784,16 @@ static ssize_t ext4_quota_read(struct super_block *sb,
int type, char *data,
static ssize_t ext4_quota_write(struct super_block *sb, int type,
const char *data, size_t len, loff_t off)
{
- struct inode *inode = sb_dqopt(sb)->files[type];
+ struct inode *inode;
ext4_lblk_t blk = off >> EXT4_BLOCK_SIZE_BITS(sb);
int err, offset = off & (sb->s_blocksize - 1);
int retries = 0;
struct buffer_head *bh;
handle_t *handle = journal_current_handle();
+ type = array_index_nospec(type, MAXQUOTAS);
+ inode = sb_dqopt(sb)->files[type];
+
if (EXT4_SB(sb)->s_journal && !handle) {
ext4_msg(sb, KERN_WARNING, "Quota write (off=%llu, len=%llu)"
" cancelled because transaction is not started",
--
2.17.1
[PATCH 1/3] ext4: super: Fix spectre gadget in ext4_quota_on
'type' is a user-controlled value used to index into 's_qf_names', which
can be used in a Spectre v1 attack. Clamp 'type' to the size of the
array to avoid a speculative out-of-bounds read.
Cc: Josh Poimboeuf
Cc: sta...@vger.kernel.org
Signed-off-by: Jeremy Cline
---
fs/ext4/super.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 6480e763080f..c04a09b51742 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -40,6 +40,7 @@
#include
#include
#include
+#include
#include
#include
@@ -5559,6 +5560,7 @@ static int ext4_quota_on(struct super_block *sb, int
type, int format_id,
if (path->dentry->d_sb != sb)
return -EXDEV;
/* Journaling quota? */
+ type = array_index_nospec(type, EXT4_MAXQUOTAS);
if (EXT4_SB(sb)->s_qf_names[type]) {
/* Quotafile not in fs root? */
if (path->dentry->d_parent != sb->s_root)
--
2.17.1
[PATCH 2/3] ext4: super: Fix spectre gadgets in ext4_quota_{read,write,off}
'type' is a user-controlled value used to index 'sb_dqopt(sb)->files'.
Clamp 'type' to the size of the array to avoid a speculative
out-of-bounds read.
These gadgets were found with the help of smatch:
* fs/ext4/super.c:5741 ext4_quota_read() warn: potential spectre issue
'sb_dqopt(sb)->files' [r]
* fs/ext4/super.c:5778 ext4_quota_write() warn: potential spectre issue
'sb_dqopt(sb)->files' [r]
Cc: Josh Poimboeuf
Cc: sta...@vger.kernel.org
Signed-off-by: Jeremy Cline
---
fs/ext4/super.c | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c04a09b51742..de358eba024a 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5693,10 +5693,13 @@ static int ext4_enable_quotas(struct super_block *sb)
static int ext4_quota_off(struct super_block *sb, int type)
{
- struct inode *inode = sb_dqopt(sb)->files[type];
+ struct inode *inode;
handle_t *handle;
int err;
+ type = array_index_nospec(type, MAXQUOTAS);
+ inode = sb_dqopt(sb)->files[type];
+
/* Force all delayed allocation blocks to be allocated.
* Caller already holds s_umount sem */
if (test_opt(sb, DELALLOC))
@@ -5740,13 +5743,17 @@ static int ext4_quota_off(struct super_block *sb, int
type)
static ssize_t ext4_quota_read(struct super_block *sb, int type, char *data,
size_t len, loff_t off)
{
- struct inode *inode = sb_dqopt(sb)->files[type];
+ struct inode *inode;
ext4_lblk_t blk = off >> EXT4_BLOCK_SIZE_BITS(sb);
int offset = off & (sb->s_blocksize - 1);
int tocopy;
size_t toread;
struct buffer_head *bh;
- loff_t i_size = i_size_read(inode);
+ loff_t i_size;
+
+ type = array_index_nospec(type, MAXQUOTAS);
+ inode = sb_dqopt(sb)->files[type];
+ i_size = i_size_read(inode);
if (off > i_size)
return 0;
@@ -5777,13 +5784,16 @@ static ssize_t ext4_quota_read(struct super_block *sb,
int type, char *data,
static ssize_t ext4_quota_write(struct super_block *sb, int type,
const char *data, size_t len, loff_t off)
{
- struct inode *inode = sb_dqopt(sb)->files[type];
+ struct inode *inode;
ext4_lblk_t blk = off >> EXT4_BLOCK_SIZE_BITS(sb);
int err, offset = off & (sb->s_blocksize - 1);
int retries = 0;
struct buffer_head *bh;
handle_t *handle = journal_current_handle();
+ type = array_index_nospec(type, MAXQUOTAS);
+ inode = sb_dqopt(sb)->files[type];
+
if (EXT4_SB(sb)->s_journal && !handle) {
ext4_msg(sb, KERN_WARNING, "Quota write (off=%llu, len=%llu)"
" cancelled because transaction is not started",
--
2.17.1
[PATCH 0/3] ext4: fix spectre v1 gadgets
Hi folks,
This patch set fixes a few Spectre v1 gadgets in ext4 detected with the
help of Smatch.
Note that because the speculation window is large, the policy is to stop
the speculative out-of-bounds load and not worry if the attack can be
completed with a dependent load or store[0].
[0] https://marc.info/?l=linux-kernel=152449131114778
Jeremy Cline (3):
ext4: super: Fix spectre gadget in ext4_quota_on
ext4: super: Fix spectre gadgets in ext4_quota_{read,write,off}
ext4: mballoc: Fix spectre gadget in ext4_mb_simple_scan_group
fs/ext4/mballoc.c | 2 ++
fs/ext4/super.c | 20
2 files changed, 18 insertions(+), 4 deletions(-)
--
2.17.1
[PATCH 0/3] ext4: fix spectre v1 gadgets
Hi folks,
This patch set fixes a few Spectre v1 gadgets in ext4 detected with the
help of Smatch.
Note that because the speculation window is large, the policy is to stop
the speculative out-of-bounds load and not worry if the attack can be
completed with a dependent load or store[0].
[0] https://marc.info/?l=linux-kernel=152449131114778
Jeremy Cline (3):
ext4: super: Fix spectre gadget in ext4_quota_on
ext4: super: Fix spectre gadgets in ext4_quota_{read,write,off}
ext4: mballoc: Fix spectre gadget in ext4_mb_simple_scan_group
fs/ext4/mballoc.c | 2 ++
fs/ext4/super.c | 20
2 files changed, 18 insertions(+), 4 deletions(-)
--
2.17.1
Re: [PATCH] scripts: Add Python 3 support to tracing/draw_functrace.py
On 07/25/2018 10:39 AM, Masahiro Yamada wrote:
> 2018-07-21 4:35 GMT+09:00 Jeremy Cline :
>> Use the print function. This maintains Python 2 support and should have
>> no functional change.
>>
>> Signed-off-by: Jeremy Cline
>> ---
>> scripts/tracing/draw_functrace.py | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/tracing/draw_functrace.py
>> b/scripts/tracing/draw_functrace.py
>> index db40fa04cd51..7d44e796d362 100755
>> --- a/scripts/tracing/draw_functrace.py
>> +++ b/scripts/tracing/draw_functrace.py
>> @@ -20,6 +20,7 @@ Usage:
>> $ scripts/draw_functrace.py < raw_trace_func > draw_functrace
>> Then you have your drawn trace in draw_functrace
>> """
>> +from __future__ import print_function
>
> What do you need this line for?
>
> I have not tested this,
> but I guess print(CallTree.ROOT) will work for Python 2.
Although "print(CallTree.ROOT)" (as a statement) works in Python 2,
its behavior is different than print (as a function) in Python 3. In
this case, there's no additional arguments being provided so the
behavior will match, but if someone added an argument it would work
differently on Python 2 vs Python 3:
Python 2.7.15
>>> print("hello", "world")
('hello', 'world')
Python 3.6.6
>>> print("hello, "world")
File "", line 1
print("hello, "world")
^
SyntaxError: invalid syntax
Importing the print_function works on Python 2.6+[0] and changes print
to be a function in Python 2 so it'll behave the same in 2 and 3. Given
that this script doesn't appear to change much it's probably not going
to save anyone from making that mistake, though. Would you prefer a
patch without it?
[0] https://docs.python.org/3/library/__future__.html
Regards,
Jeremy
>
>
>
>>
>> import sys, re
>> @@ -123,7 +124,7 @@ def main():
>> tree = tree.getParent(caller)
>> tree = tree.calls(callee, calltime)
>>
>> - print CallTree.ROOT
>> + print(CallTree.ROOT)
>>
>> if __name__ == "__main__":
>> main()
>> --
>> 2.17.1
>>
>
>
>
Re: [PATCH] scripts: Add Python 3 support to tracing/draw_functrace.py
On 07/25/2018 10:39 AM, Masahiro Yamada wrote:
> 2018-07-21 4:35 GMT+09:00 Jeremy Cline :
>> Use the print function. This maintains Python 2 support and should have
>> no functional change.
>>
>> Signed-off-by: Jeremy Cline
>> ---
>> scripts/tracing/draw_functrace.py | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/tracing/draw_functrace.py
>> b/scripts/tracing/draw_functrace.py
>> index db40fa04cd51..7d44e796d362 100755
>> --- a/scripts/tracing/draw_functrace.py
>> +++ b/scripts/tracing/draw_functrace.py
>> @@ -20,6 +20,7 @@ Usage:
>> $ scripts/draw_functrace.py < raw_trace_func > draw_functrace
>> Then you have your drawn trace in draw_functrace
>> """
>> +from __future__ import print_function
>
> What do you need this line for?
>
> I have not tested this,
> but I guess print(CallTree.ROOT) will work for Python 2.
Although "print(CallTree.ROOT)" (as a statement) works in Python 2,
its behavior is different than print (as a function) in Python 3. In
this case, there's no additional arguments being provided so the
behavior will match, but if someone added an argument it would work
differently on Python 2 vs Python 3:
Python 2.7.15
>>> print("hello", "world")
('hello', 'world')
Python 3.6.6
>>> print("hello, "world")
File "", line 1
print("hello, "world")
^
SyntaxError: invalid syntax
Importing the print_function works on Python 2.6+[0] and changes print
to be a function in Python 2 so it'll behave the same in 2 and 3. Given
that this script doesn't appear to change much it's probably not going
to save anyone from making that mistake, though. Would you prefer a
patch without it?
[0] https://docs.python.org/3/library/__future__.html
Regards,
Jeremy
>
>
>
>>
>> import sys, re
>> @@ -123,7 +124,7 @@ def main():
>> tree = tree.getParent(caller)
>> tree = tree.calls(callee, calltime)
>>
>> - print CallTree.ROOT
>> + print(CallTree.ROOT)
>>
>> if __name__ == "__main__":
>> main()
>> --
>> 2.17.1
>>
>
>
>
[PATCH] scripts: Add Python 3 support to tracing/draw_functrace.py
Use the print function. This maintains Python 2 support and should have no functional change. Signed-off-by: Jeremy Cline --- scripts/tracing/draw_functrace.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/tracing/draw_functrace.py b/scripts/tracing/draw_functrace.py index db40fa04cd51..7d44e796d362 100755 --- a/scripts/tracing/draw_functrace.py +++ b/scripts/tracing/draw_functrace.py @@ -20,6 +20,7 @@ Usage: $ scripts/draw_functrace.py < raw_trace_func > draw_functrace Then you have your drawn trace in draw_functrace """ +from __future__ import print_function import sys, re @@ -123,7 +124,7 @@ def main(): tree = tree.getParent(caller) tree = tree.calls(callee, calltime) - print CallTree.ROOT + print(CallTree.ROOT) if __name__ == "__main__": main() -- 2.17.1
[PATCH] scripts: Add Python 3 support to tracing/draw_functrace.py
Use the print function. This maintains Python 2 support and should have no functional change. Signed-off-by: Jeremy Cline --- scripts/tracing/draw_functrace.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/tracing/draw_functrace.py b/scripts/tracing/draw_functrace.py index db40fa04cd51..7d44e796d362 100755 --- a/scripts/tracing/draw_functrace.py +++ b/scripts/tracing/draw_functrace.py @@ -20,6 +20,7 @@ Usage: $ scripts/draw_functrace.py < raw_trace_func > draw_functrace Then you have your drawn trace in draw_functrace """ +from __future__ import print_function import sys, re @@ -123,7 +124,7 @@ def main(): tree = tree.getParent(caller) tree = tree.calls(callee, calltime) - print CallTree.ROOT + print(CallTree.ROOT) if __name__ == "__main__": main() -- 2.17.1
[PATCH] bpf: Add Python 3 support to selftests scripts for bpf
Adjust tcp_client.py and tcp_server.py to work with Python 3 by using
the print function, marking string literals as bytes, and using the
newer exception syntax. This should be functionally equivalent and
support Python 2.6 through Python 3.7.
Signed-off-by: Jeremy Cline
---
tools/testing/selftests/bpf/tcp_client.py | 12 ++--
tools/testing/selftests/bpf/tcp_server.py | 17 +
2 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/bpf/tcp_client.py
b/tools/testing/selftests/bpf/tcp_client.py
index 481dccdf140c..9fe5f1b5c020 100755
--- a/tools/testing/selftests/bpf/tcp_client.py
+++ b/tools/testing/selftests/bpf/tcp_client.py
@@ -1,4 +1,4 @@
-#!/usr/bin/env python2
+#!/usr/bin/env python
#
# SPDX-License-Identifier: GPL-2.0
#
@@ -9,11 +9,11 @@ import subprocess
import select
def read(sock, n):
-buf = ''
+buf = b''
while len(buf) < n:
rem = n - len(buf)
try: s = sock.recv(rem)
-except (socket.error), e: return ''
+except (socket.error) as e: return b''
buf += s
return buf
@@ -22,7 +22,7 @@ def send(sock, s):
count = 0
while count < total:
try: n = sock.send(s)
-except (socket.error), e: n = 0
+except (socket.error) as e: n = 0
if n == 0:
return count;
count += n
@@ -39,10 +39,10 @@ try:
except socket.error as e:
sys.exit(1)
-buf = ''
+buf = b''
n = 0
while n < 1000:
-buf += '+'
+buf += b'+'
n += 1
sock.settimeout(1);
diff --git a/tools/testing/selftests/bpf/tcp_server.py
b/tools/testing/selftests/bpf/tcp_server.py
index bc454d7d0be2..1d4a40a6584b 100755
--- a/tools/testing/selftests/bpf/tcp_server.py
+++ b/tools/testing/selftests/bpf/tcp_server.py
@@ -1,7 +1,8 @@
-#!/usr/bin/env python2
+#!/usr/bin/env python
#
# SPDX-License-Identifier: GPL-2.0
#
+from __future__ import print_function
import sys, os, os.path, getopt
import socket, time
@@ -9,11 +10,11 @@ import subprocess
import select
def read(sock, n):
-buf = ''
+buf = b''
while len(buf) < n:
rem = n - len(buf)
try: s = sock.recv(rem)
-except (socket.error), e: return ''
+except (socket.error) as e: return b''
buf += s
return buf
@@ -22,7 +23,7 @@ def send(sock, s):
count = 0
while count < total:
try: n = sock.send(s)
-except (socket.error), e: n = 0
+except (socket.error) as e: n = 0
if n == 0:
return count;
count += n
@@ -43,7 +44,7 @@ host = socket.gethostname()
try: serverSocket.bind((host, 0))
except socket.error as msg:
-print 'bind fails: ', msg
+print('bind fails: ' + str(msg))
sn = serverSocket.getsockname()
serverPort = sn[1]
@@ -51,10 +52,10 @@ serverPort = sn[1]
cmdStr = ("./tcp_client.py %d &") % (serverPort)
os.system(cmdStr)
-buf = ''
+buf = b''
n = 0
while n < 500:
-buf += '.'
+buf += b'.'
n += 1
serverSocket.listen(MAX_PORTS)
@@ -79,5 +80,5 @@ while True:
serverSocket.close()
sys.exit(0)
else:
-print 'Select timeout!'
+print('Select timeout!')
sys.exit(1)
--
2.17.1
[PATCH] bpf: Add Python 3 support to selftests scripts for bpf
Adjust tcp_client.py and tcp_server.py to work with Python 3 by using
the print function, marking string literals as bytes, and using the
newer exception syntax. This should be functionally equivalent and
support Python 2.6 through Python 3.7.
Signed-off-by: Jeremy Cline
---
tools/testing/selftests/bpf/tcp_client.py | 12 ++--
tools/testing/selftests/bpf/tcp_server.py | 17 +
2 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/bpf/tcp_client.py
b/tools/testing/selftests/bpf/tcp_client.py
index 481dccdf140c..9fe5f1b5c020 100755
--- a/tools/testing/selftests/bpf/tcp_client.py
+++ b/tools/testing/selftests/bpf/tcp_client.py
@@ -1,4 +1,4 @@
-#!/usr/bin/env python2
+#!/usr/bin/env python
#
# SPDX-License-Identifier: GPL-2.0
#
@@ -9,11 +9,11 @@ import subprocess
import select
def read(sock, n):
-buf = ''
+buf = b''
while len(buf) < n:
rem = n - len(buf)
try: s = sock.recv(rem)
-except (socket.error), e: return ''
+except (socket.error) as e: return b''
buf += s
return buf
@@ -22,7 +22,7 @@ def send(sock, s):
count = 0
while count < total:
try: n = sock.send(s)
-except (socket.error), e: n = 0
+except (socket.error) as e: n = 0
if n == 0:
return count;
count += n
@@ -39,10 +39,10 @@ try:
except socket.error as e:
sys.exit(1)
-buf = ''
+buf = b''
n = 0
while n < 1000:
-buf += '+'
+buf += b'+'
n += 1
sock.settimeout(1);
diff --git a/tools/testing/selftests/bpf/tcp_server.py
b/tools/testing/selftests/bpf/tcp_server.py
index bc454d7d0be2..1d4a40a6584b 100755
--- a/tools/testing/selftests/bpf/tcp_server.py
+++ b/tools/testing/selftests/bpf/tcp_server.py
@@ -1,7 +1,8 @@
-#!/usr/bin/env python2
+#!/usr/bin/env python
#
# SPDX-License-Identifier: GPL-2.0
#
+from __future__ import print_function
import sys, os, os.path, getopt
import socket, time
@@ -9,11 +10,11 @@ import subprocess
import select
def read(sock, n):
-buf = ''
+buf = b''
while len(buf) < n:
rem = n - len(buf)
try: s = sock.recv(rem)
-except (socket.error), e: return ''
+except (socket.error) as e: return b''
buf += s
return buf
@@ -22,7 +23,7 @@ def send(sock, s):
count = 0
while count < total:
try: n = sock.send(s)
-except (socket.error), e: n = 0
+except (socket.error) as e: n = 0
if n == 0:
return count;
count += n
@@ -43,7 +44,7 @@ host = socket.gethostname()
try: serverSocket.bind((host, 0))
except socket.error as msg:
-print 'bind fails: ', msg
+print('bind fails: ' + str(msg))
sn = serverSocket.getsockname()
serverPort = sn[1]
@@ -51,10 +52,10 @@ serverPort = sn[1]
cmdStr = ("./tcp_client.py %d &") % (serverPort)
os.system(cmdStr)
-buf = ''
+buf = b''
n = 0
while n < 500:
-buf += '.'
+buf += b'.'
n += 1
serverSocket.listen(MAX_PORTS)
@@ -79,5 +80,5 @@ while True:
serverSocket.close()
sys.exit(0)
else:
-print 'Select timeout!'
+print('Select timeout!')
sys.exit(1)
--
2.17.1
[PATCH] scripts: Add Python 3 compatibility to spdxcheck.py
"dict.has_key(key)" on dictionaries has been replaced with "key in
dict". Additionally, when run under Python 3 some files don't decode
with the default encoding (tested with UTF-8). To handle that, don't
open the file in text mode and decode text line-by-line, ignoring
encoding errors.
This remains compatible with Python 2 and should have no functional
change.
Signed-off-by: Jeremy Cline
---
scripts/spdxcheck.py | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/scripts/spdxcheck.py b/scripts/spdxcheck.py
index a6041f29b18e..839e190bbd7a 100755
--- a/scripts/spdxcheck.py
+++ b/scripts/spdxcheck.py
@@ -4,6 +4,7 @@
from argparse import ArgumentParser
from ply import lex, yacc
+import locale
import traceback
import sys
import git
@@ -102,7 +103,7 @@ class id_parser(object):
raise ParserException(tok, 'Invalid License ID')
self.lastid = id
elif tok.type == 'EXC':
-if not self.spdx.exceptions.has_key(id):
+if id not in self.spdx.exceptions:
raise ParserException(tok, 'Invalid Exception ID')
if self.lastid not in self.spdx.exceptions[id]:
raise ParserException(tok, 'Exception not valid for license
%s' %self.lastid)
@@ -167,6 +168,7 @@ class id_parser(object):
self.curline = 0
try:
for line in fd:
+line = line.decode(locale.getpreferredencoding(False),
errors='ignore')
self.curline += 1
if self.curline > maxlines:
break
@@ -201,7 +203,8 @@ def scan_git_tree(tree):
continue
if not os.path.isfile(el.path):
continue
-parser.parse_lines(open(el.path), args.maxlines, el.path)
+with open(el.path, 'rb') as fd:
+parser.parse_lines(fd, args.maxlines, el.path)
def scan_git_subtree(tree, path):
for p in path.strip('/').split('/'):
--
2.17.1
[PATCH] scripts: Add Python 3 compatibility to spdxcheck.py
"dict.has_key(key)" on dictionaries has been replaced with "key in
dict". Additionally, when run under Python 3 some files don't decode
with the default encoding (tested with UTF-8). To handle that, don't
open the file in text mode and decode text line-by-line, ignoring
encoding errors.
This remains compatible with Python 2 and should have no functional
change.
Signed-off-by: Jeremy Cline
---
scripts/spdxcheck.py | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/scripts/spdxcheck.py b/scripts/spdxcheck.py
index a6041f29b18e..839e190bbd7a 100755
--- a/scripts/spdxcheck.py
+++ b/scripts/spdxcheck.py
@@ -4,6 +4,7 @@
from argparse import ArgumentParser
from ply import lex, yacc
+import locale
import traceback
import sys
import git
@@ -102,7 +103,7 @@ class id_parser(object):
raise ParserException(tok, 'Invalid License ID')
self.lastid = id
elif tok.type == 'EXC':
-if not self.spdx.exceptions.has_key(id):
+if id not in self.spdx.exceptions:
raise ParserException(tok, 'Invalid Exception ID')
if self.lastid not in self.spdx.exceptions[id]:
raise ParserException(tok, 'Exception not valid for license
%s' %self.lastid)
@@ -167,6 +168,7 @@ class id_parser(object):
self.curline = 0
try:
for line in fd:
+line = line.decode(locale.getpreferredencoding(False),
errors='ignore')
self.curline += 1
if self.curline > maxlines:
break
@@ -201,7 +203,8 @@ def scan_git_tree(tree):
continue
if not os.path.isfile(el.path):
continue
-parser.parse_lines(open(el.path), args.maxlines, el.path)
+with open(el.path, 'rb') as fd:
+parser.parse_lines(fd, args.maxlines, el.path)
def scan_git_subtree(tree, path):
for p in path.strip('/').split('/'):
--
2.17.1
[tip:perf/urgent] perf tools: Use python-config --includes rather than --cflags
Commit-ID: 32aa928a7b817140c84987b726d5014911808fa4
Gitweb: https://git.kernel.org/tip/32aa928a7b817140c84987b726d5014911808fa4
Author: Jeremy Cline
AuthorDate: Tue, 10 Jul 2018 11:46:12 -0400
Committer: Arnaldo Carvalho de Melo
CommitDate: Wed, 11 Jul 2018 09:48:31 -0400
perf tools: Use python-config --includes rather than --cflags
Builds started failing in Fedora on Python 3.7 with:
`.gnu.debuglto_.debug_macro' referenced in section
`.gnu.debuglto_.debug_macro' of
util/scripting-engines/trace-event-python.o: defined in discarded
section
In Fedora, Python 3.7 added -flto to the list of --cflags and since it
was only applied to util/scripting-engines/trace-event-python.c and
scripts/python/Perf-Trace-Util/Context.c, linking failed.
It's not the first time the addition of flags has broken builds: commit
c6707fdef7e2 ("perf tools: Fix up build in hardnened environments")
appears to have fixed a similar problem. "python-config --includes"
provides the proper -I flags and doesn't introduce additional CFLAGS.
Signed-off-by: Jeremy Cline
Cc: Alexander Shishkin
Cc: Jiri Olsa
Cc: Namhyung Kim
Cc: Peter Zijlstra
Link: http://lkml.kernel.org/r/20180710154612.6285-1-jcl...@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo
---
tools/perf/Makefile.config | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index b5ac356ba323..f5a3b402589e 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -207,8 +207,7 @@ ifdef PYTHON_CONFIG
PYTHON_EMBED_LDOPTS := $(shell $(PYTHON_CONFIG_SQ) --ldflags 2>/dev/null)
PYTHON_EMBED_LDFLAGS := $(call strip-libs,$(PYTHON_EMBED_LDOPTS))
PYTHON_EMBED_LIBADD := $(call grep-libs,$(PYTHON_EMBED_LDOPTS)) -lutil
- PYTHON_EMBED_CCOPTS := $(shell $(PYTHON_CONFIG_SQ) --cflags 2>/dev/null)
- PYTHON_EMBED_CCOPTS := $(filter-out -specs=%,$(PYTHON_EMBED_CCOPTS))
+ PYTHON_EMBED_CCOPTS := $(shell $(PYTHON_CONFIG_SQ) --includes 2>/dev/null)
FLAGS_PYTHON_EMBED := $(PYTHON_EMBED_CCOPTS) $(PYTHON_EMBED_LDOPTS)
endif
[tip:perf/urgent] perf tools: Use python-config --includes rather than --cflags
Commit-ID: 32aa928a7b817140c84987b726d5014911808fa4
Gitweb: https://git.kernel.org/tip/32aa928a7b817140c84987b726d5014911808fa4
Author: Jeremy Cline
AuthorDate: Tue, 10 Jul 2018 11:46:12 -0400
Committer: Arnaldo Carvalho de Melo
CommitDate: Wed, 11 Jul 2018 09:48:31 -0400
perf tools: Use python-config --includes rather than --cflags
Builds started failing in Fedora on Python 3.7 with:
`.gnu.debuglto_.debug_macro' referenced in section
`.gnu.debuglto_.debug_macro' of
util/scripting-engines/trace-event-python.o: defined in discarded
section
In Fedora, Python 3.7 added -flto to the list of --cflags and since it
was only applied to util/scripting-engines/trace-event-python.c and
scripts/python/Perf-Trace-Util/Context.c, linking failed.
It's not the first time the addition of flags has broken builds: commit
c6707fdef7e2 ("perf tools: Fix up build in hardnened environments")
appears to have fixed a similar problem. "python-config --includes"
provides the proper -I flags and doesn't introduce additional CFLAGS.
Signed-off-by: Jeremy Cline
Cc: Alexander Shishkin
Cc: Jiri Olsa
Cc: Namhyung Kim
Cc: Peter Zijlstra
Link: http://lkml.kernel.org/r/20180710154612.6285-1-jcl...@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo
---
tools/perf/Makefile.config | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index b5ac356ba323..f5a3b402589e 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -207,8 +207,7 @@ ifdef PYTHON_CONFIG
PYTHON_EMBED_LDOPTS := $(shell $(PYTHON_CONFIG_SQ) --ldflags 2>/dev/null)
PYTHON_EMBED_LDFLAGS := $(call strip-libs,$(PYTHON_EMBED_LDOPTS))
PYTHON_EMBED_LIBADD := $(call grep-libs,$(PYTHON_EMBED_LDOPTS)) -lutil
- PYTHON_EMBED_CCOPTS := $(shell $(PYTHON_CONFIG_SQ) --cflags 2>/dev/null)
- PYTHON_EMBED_CCOPTS := $(filter-out -specs=%,$(PYTHON_EMBED_CCOPTS))
+ PYTHON_EMBED_CCOPTS := $(shell $(PYTHON_CONFIG_SQ) --includes 2>/dev/null)
FLAGS_PYTHON_EMBED := $(PYTHON_EMBED_CCOPTS) $(PYTHON_EMBED_LDOPTS)
endif
[tip:perf/urgent] perf scripts python: Add Python 3 support to EventClass.py
Commit-ID: 12aa6c7389a321b4b5d921f89c3f83b9750598f7
Gitweb: https://git.kernel.org/tip/12aa6c7389a321b4b5d921f89c3f83b9750598f7
Author: Jeremy Cline
AuthorDate: Tue, 8 May 2018 21:27:48 +
Committer: Arnaldo Carvalho de Melo
CommitDate: Wed, 11 Jul 2018 10:01:50 -0300
perf scripts python: Add Python 3 support to EventClass.py
Support both Python 2 and Python 3 in EventClass.py. ``print`` is now a
function rather than a statement. This should have no functional change.
Signed-off-by: Jeremy Cline
Cc: Alexander Shishkin
Cc: Herton Krzesinski
Cc: Jiri Olsa
Cc: Namhyung Kim
Cc: Peter Zijlstra
Link:
http://lkml.kernel.org/r/0100016341a73aac-e0734bdc-dcab-4c61-8333-d8be97524aa0-000...@email.amazonses.com
Signed-off-by: Arnaldo Carvalho de Melo
---
.../perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/EventClass.py | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git
a/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/EventClass.py
b/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/EventClass.py
index 81a56cd2b3c1..21a7a1298094 100755
--- a/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/EventClass.py
+++ b/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/EventClass.py
@@ -8,6 +8,7 @@
# PerfEvent is the base class for all perf event sample, PebsEvent
# is a HW base Intel x86 PEBS event, and user could add more SW/HW
# event classes based on requirements.
+from __future__ import print_function
import struct
@@ -44,7 +45,8 @@ class PerfEvent(object):
PerfEvent.event_num += 1
def show(self):
-print "PMU event: name=%12s, symbol=%24s, comm=%8s, dso=%12s"
% (self.name, self.symbol, self.comm, self.dso)
+print("PMU event: name=%12s, symbol=%24s, comm=%8s, dso=%12s" %
+ (self.name, self.symbol, self.comm, self.dso))
#
# Basic Intel PEBS (Precise Event-based Sampling) event, whose raw buffer
[tip:perf/urgent] perf scripts python: Add Python 3 support to EventClass.py
Commit-ID: 12aa6c7389a321b4b5d921f89c3f83b9750598f7
Gitweb: https://git.kernel.org/tip/12aa6c7389a321b4b5d921f89c3f83b9750598f7
Author: Jeremy Cline
AuthorDate: Tue, 8 May 2018 21:27:48 +
Committer: Arnaldo Carvalho de Melo
CommitDate: Wed, 11 Jul 2018 10:01:50 -0300
perf scripts python: Add Python 3 support to EventClass.py
Support both Python 2 and Python 3 in EventClass.py. ``print`` is now a
function rather than a statement. This should have no functional change.
Signed-off-by: Jeremy Cline
Cc: Alexander Shishkin
Cc: Herton Krzesinski
Cc: Jiri Olsa
Cc: Namhyung Kim
Cc: Peter Zijlstra
Link:
http://lkml.kernel.org/r/0100016341a73aac-e0734bdc-dcab-4c61-8333-d8be97524aa0-000...@email.amazonses.com
Signed-off-by: Arnaldo Carvalho de Melo
---
.../perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/EventClass.py | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git
a/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/EventClass.py
b/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/EventClass.py
index 81a56cd2b3c1..21a7a1298094 100755
--- a/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/EventClass.py
+++ b/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/EventClass.py
@@ -8,6 +8,7 @@
# PerfEvent is the base class for all perf event sample, PebsEvent
# is a HW base Intel x86 PEBS event, and user could add more SW/HW
# event classes based on requirements.
+from __future__ import print_function
import struct
@@ -44,7 +45,8 @@ class PerfEvent(object):
PerfEvent.event_num += 1
def show(self):
-print "PMU event: name=%12s, symbol=%24s, comm=%8s, dso=%12s"
% (self.name, self.symbol, self.comm, self.dso)
+print("PMU event: name=%12s, symbol=%24s, comm=%8s, dso=%12s" %
+ (self.name, self.symbol, self.comm, self.dso))
#
# Basic Intel PEBS (Precise Event-based Sampling) event, whose raw buffer
[tip:perf/urgent] perf scripts python: Add Python 3 support to sched-migration.py
Commit-ID: 8c1c1ab2d2a77cb50841822168e56d11c4ebfd6e
Gitweb: https://git.kernel.org/tip/8c1c1ab2d2a77cb50841822168e56d11c4ebfd6e
Author: Jeremy Cline
AuthorDate: Tue, 8 May 2018 21:27:47 +
Committer: Arnaldo Carvalho de Melo
CommitDate: Wed, 11 Jul 2018 10:01:50 -0300
perf scripts python: Add Python 3 support to sched-migration.py
Support both Python 2 and Python 3 in the sched-migration.py script.
This should have no functional change.
Signed-off-by: Jeremy Cline
Cc: Alexander Shishkin
Cc: Herton Krzesinski
Cc: Jiri Olsa
Cc: Namhyung Kim
Cc: Peter Zijlstra
Link:
http://lkml.kernel.org/r/0100016341a737a5-44ec436f-3440-4cac-a03f-ddfa589bf308-000...@email.amazonses.com
Signed-off-by: Arnaldo Carvalho de Melo
---
tools/perf/scripts/python/sched-migration.py | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/tools/perf/scripts/python/sched-migration.py
b/tools/perf/scripts/python/sched-migration.py
index de66cb3b72c9..3473e7f66081 100644
--- a/tools/perf/scripts/python/sched-migration.py
+++ b/tools/perf/scripts/python/sched-migration.py
@@ -9,13 +9,17 @@
# This software is distributed under the terms of the GNU General
# Public License ("GPL") version 2 as published by the Free Software
# Foundation.
-
+from __future__ import print_function
import os
import sys
from collections import defaultdict
-from UserList import UserList
+try:
+from UserList import UserList
+except ImportError:
+# Python 3: UserList moved to the collections package
+from collections import UserList
sys.path.append(os.environ['PERF_EXEC_PATH'] + \
'/scripts/python/Perf-Trace-Util/lib/Perf/Trace')
@@ -300,7 +304,7 @@ class TimeSliceList(UserList):
if i == -1:
return
- for i in xrange(i, len(self.data)):
+ for i in range(i, len(self.data)):
timeslice = self.data[i]
if timeslice.start > end:
return
@@ -336,8 +340,8 @@ class SchedEventProxy:
on_cpu_task = self.current_tsk[headers.cpu]
if on_cpu_task != -1 and on_cpu_task != prev_pid:
- print "Sched switch event rejected ts: %s cpu: %d prev:
%s(%d) next: %s(%d)" % \
- (headers.ts_format(), headers.cpu, prev_comm,
prev_pid, next_comm, next_pid)
+ print("Sched switch event rejected ts: %s cpu: %d prev:
%s(%d) next: %s(%d)" % \
+ headers.ts_format(), headers.cpu, prev_comm,
prev_pid, next_comm, next_pid)
threads[prev_pid] = prev_comm
threads[next_pid] = next_comm
[tip:perf/urgent] perf scripts python: Add Python 3 support to sched-migration.py
Commit-ID: 8c1c1ab2d2a77cb50841822168e56d11c4ebfd6e
Gitweb: https://git.kernel.org/tip/8c1c1ab2d2a77cb50841822168e56d11c4ebfd6e
Author: Jeremy Cline
AuthorDate: Tue, 8 May 2018 21:27:47 +
Committer: Arnaldo Carvalho de Melo
CommitDate: Wed, 11 Jul 2018 10:01:50 -0300
perf scripts python: Add Python 3 support to sched-migration.py
Support both Python 2 and Python 3 in the sched-migration.py script.
This should have no functional change.
Signed-off-by: Jeremy Cline
Cc: Alexander Shishkin
Cc: Herton Krzesinski
Cc: Jiri Olsa
Cc: Namhyung Kim
Cc: Peter Zijlstra
Link:
http://lkml.kernel.org/r/0100016341a737a5-44ec436f-3440-4cac-a03f-ddfa589bf308-000...@email.amazonses.com
Signed-off-by: Arnaldo Carvalho de Melo
---
tools/perf/scripts/python/sched-migration.py | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/tools/perf/scripts/python/sched-migration.py
b/tools/perf/scripts/python/sched-migration.py
index de66cb3b72c9..3473e7f66081 100644
--- a/tools/perf/scripts/python/sched-migration.py
+++ b/tools/perf/scripts/python/sched-migration.py
@@ -9,13 +9,17 @@
# This software is distributed under the terms of the GNU General
# Public License ("GPL") version 2 as published by the Free Software
# Foundation.
-
+from __future__ import print_function
import os
import sys
from collections import defaultdict
-from UserList import UserList
+try:
+from UserList import UserList
+except ImportError:
+# Python 3: UserList moved to the collections package
+from collections import UserList
sys.path.append(os.environ['PERF_EXEC_PATH'] + \
'/scripts/python/Perf-Trace-Util/lib/Perf/Trace')
@@ -300,7 +304,7 @@ class TimeSliceList(UserList):
if i == -1:
return
- for i in xrange(i, len(self.data)):
+ for i in range(i, len(self.data)):
timeslice = self.data[i]
if timeslice.start > end:
return
@@ -336,8 +340,8 @@ class SchedEventProxy:
on_cpu_task = self.current_tsk[headers.cpu]
if on_cpu_task != -1 and on_cpu_task != prev_pid:
- print "Sched switch event rejected ts: %s cpu: %d prev:
%s(%d) next: %s(%d)" % \
- (headers.ts_format(), headers.cpu, prev_comm,
prev_pid, next_comm, next_pid)
+ print("Sched switch event rejected ts: %s cpu: %d prev:
%s(%d) next: %s(%d)" % \
+ headers.ts_format(), headers.cpu, prev_comm,
prev_pid, next_comm, next_pid)
threads[prev_pid] = prev_comm
threads[next_pid] = next_comm
[tip:perf/urgent] perf scripts python: Add Python 3 support to Util.py
Commit-ID: c45b168effb12719f6dfc8d2c20ba8c057e8c16b
Gitweb: https://git.kernel.org/tip/c45b168effb12719f6dfc8d2c20ba8c057e8c16b
Author: Jeremy Cline
AuthorDate: Tue, 8 May 2018 21:27:46 +
Committer: Arnaldo Carvalho de Melo
CommitDate: Wed, 11 Jul 2018 10:01:50 -0300
perf scripts python: Add Python 3 support to Util.py
Support both Python 2 and Python 3 in Util.py. The dict class no longer
has a ``has_key`` method and print is now a function rather than a
statement. This should have no functional change.
Signed-off-by: Jeremy Cline
Cc: Alexander Shishkin
Cc: Herton Krzesinski
Cc: Jiri Olsa
Cc: Namhyung Kim
Cc: Peter Zijlstra
Link:
http://lkml.kernel.org/r/0100016341a730c6-8db8b9b1-da2d-4ee3-96bf-47e0ae9796bd-000...@email.amazonses.com
Signed-off-by: Arnaldo Carvalho de Melo
---
.../scripts/python/Perf-Trace-Util/lib/Perf/Trace/Util.py | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/Util.py
b/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/Util.py
index f6c84966e4f8..7384dcb628c4 100644
--- a/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/Util.py
+++ b/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/Util.py
@@ -5,6 +5,7 @@
# This software may be distributed under the terms of the GNU General
# Public License ("GPL") version 2 as published by the Free Software
# Foundation.
+from __future__ import print_function
import errno, os
@@ -33,7 +34,7 @@ def nsecs_str(nsecs):
return str
def add_stats(dict, key, value):
- if not dict.has_key(key):
+ if key not in dict:
dict[key] = (value, value, value, 1)
else:
min, max, avg, count = dict[key]
@@ -72,10 +73,10 @@ try:
except:
if not audit_package_warned:
audit_package_warned = True
- print "Install the audit-libs-python package to get syscall
names.\n" \
-"For example:\n # apt-get install python-audit (Ubuntu)" \
-"\n # yum install audit-libs-python (Fedora)" \
-"\n etc.\n"
+ print("Install the audit-libs-python package to get syscall
names.\n"
+"For example:\n # apt-get install python-audit (Ubuntu)"
+"\n # yum install audit-libs-python (Fedora)"
+"\n etc.\n")
def syscall_name(id):
try:
[tip:perf/urgent] perf scripts python: Add Python 3 support to Util.py
Commit-ID: c45b168effb12719f6dfc8d2c20ba8c057e8c16b
Gitweb: https://git.kernel.org/tip/c45b168effb12719f6dfc8d2c20ba8c057e8c16b
Author: Jeremy Cline
AuthorDate: Tue, 8 May 2018 21:27:46 +
Committer: Arnaldo Carvalho de Melo
CommitDate: Wed, 11 Jul 2018 10:01:50 -0300
perf scripts python: Add Python 3 support to Util.py
Support both Python 2 and Python 3 in Util.py. The dict class no longer
has a ``has_key`` method and print is now a function rather than a
statement. This should have no functional change.
Signed-off-by: Jeremy Cline
Cc: Alexander Shishkin
Cc: Herton Krzesinski
Cc: Jiri Olsa
Cc: Namhyung Kim
Cc: Peter Zijlstra
Link:
http://lkml.kernel.org/r/0100016341a730c6-8db8b9b1-da2d-4ee3-96bf-47e0ae9796bd-000...@email.amazonses.com
Signed-off-by: Arnaldo Carvalho de Melo
---
.../scripts/python/Perf-Trace-Util/lib/Perf/Trace/Util.py | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/Util.py
b/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/Util.py
index f6c84966e4f8..7384dcb628c4 100644
--- a/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/Util.py
+++ b/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/Util.py
@@ -5,6 +5,7 @@
# This software may be distributed under the terms of the GNU General
# Public License ("GPL") version 2 as published by the Free Software
# Foundation.
+from __future__ import print_function
import errno, os
@@ -33,7 +34,7 @@ def nsecs_str(nsecs):
return str
def add_stats(dict, key, value):
- if not dict.has_key(key):
+ if key not in dict:
dict[key] = (value, value, value, 1)
else:
min, max, avg, count = dict[key]
@@ -72,10 +73,10 @@ try:
except:
if not audit_package_warned:
audit_package_warned = True
- print "Install the audit-libs-python package to get syscall
names.\n" \
-"For example:\n # apt-get install python-audit (Ubuntu)" \
-"\n # yum install audit-libs-python (Fedora)" \
-"\n etc.\n"
+ print("Install the audit-libs-python package to get syscall
names.\n"
+"For example:\n # apt-get install python-audit (Ubuntu)"
+"\n # yum install audit-libs-python (Fedora)"
+"\n etc.\n")
def syscall_name(id):
try:
[tip:perf/urgent] perf tools: Generate a Python script compatible with Python 2 and 3
Commit-ID: 877cc639686b68c7de179a485544f4761e376b30
Gitweb: https://git.kernel.org/tip/877cc639686b68c7de179a485544f4761e376b30
Author: Jeremy Cline
AuthorDate: Tue, 8 May 2018 21:27:43 +
Committer: Arnaldo Carvalho de Melo
CommitDate: Wed, 11 Jul 2018 10:01:50 -0300
perf tools: Generate a Python script compatible with Python 2 and 3
When generating a Python script with "perf script -g python", produce
one that is compatible with Python 2 and 3. The difference between the
two generated scripts is:
--- python2-perf-script.py2018-05-08 15:35:00.865889705 -0400
+++ python3-perf-script.py2018-05-08 15:34:49.019789564 -0400
@@ -7,6 +7,8 @@
# be retrieved using Python functions of the form common_*(context).
# See the perf-script-python Documentation for the list of available
functions.
+from __future__ import print_function
+
import os
import sys
@@ -18,10 +20,10 @@
def trace_begin():
- print "in trace_begin"
+ print("in trace_begin")
def trace_end():
- print "in trace_end"
+ print("in trace_end")
def raw_syscalls__sys_enter(event_name, context, common_cpu,
common_secs, common_nsecs, common_pid, common_comm,
@@ -29,26 +31,26 @@
print_header(event_name, common_cpu, common_secs, common_nsecs,
common_pid, common_comm)
- print "id=%d, args=%s" % \
- (id, args)
+ print("id=%d, args=%s" % \
+ (id, args))
- print 'Sample:
{'+get_dict_as_string(perf_sample_dict['sample'], ', ')+'}'
+ print('Sample:
{'+get_dict_as_string(perf_sample_dict['sample'], ', ')+'}')
for node in common_callchain:
if 'sym' in node:
- print "\t[%x] %s" % (node['ip'],
node['sym']['name'])
+ print("\t[%x] %s" % (node['ip'],
node['sym']['name']))
else:
- print " [%x]" % (node['ip'])
+ print(" [%x]" % (node['ip']))
- print "\n"
+ print()
def trace_unhandled(event_name, context, event_fields_dict,
perf_sample_dict):
- print get_dict_as_string(event_fields_dict)
- print 'Sample:
{'+get_dict_as_string(perf_sample_dict['sample'], ', ')+'}'
+ print(get_dict_as_string(event_fields_dict))
+ print('Sample:
{'+get_dict_as_string(perf_sample_dict['sample'], ', ')+'}')
def print_header(event_name, cpu, secs, nsecs, pid, comm):
- print "%-20s %5u %05u.%09u %8u %-20s " % \
- (event_name, cpu, secs, nsecs, pid, comm),
+ print("%-20s %5u %05u.%09u %8u %-20s " % \
+ (event_name, cpu, secs, nsecs, pid, comm), end="")
def get_dict_as_string(a_dict, delimiter=' '):
return delimiter.join(['%s=%s'%(k,str(v))for k,v in
sorted(a_dict.items())])
Signed-off-by: Jeremy Cline
Cc: Alexander Shishkin
Cc: Herton Krzesinski
Cc: Jiri Olsa
Cc: Namhyung Kim
Cc: Peter Zijlstra
Link:
http://lkml.kernel.org/r/0100016341a7278a-d178c724-2b0f-49ca-be93-80a7d51aaa0d-000...@email.amazonses.com
Signed-off-by: Arnaldo Carvalho de Melo
---
.../util/scripting-engines/trace-event-python.c| 29 +++---
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/tools/perf/util/scripting-engines/trace-event-python.c
b/tools/perf/util/scripting-engines/trace-event-python.c
index 46e9e19ab1ac..8b2eb6dbff4d 100644
--- a/tools/perf/util/scripting-engines/trace-event-python.c
+++ b/tools/perf/util/scripting-engines/trace-event-python.c
@@ -1627,6 +1627,7 @@ static int python_generate_script(struct pevent *pevent,
const char *outfile)
fprintf(ofp, "# See the perf-script-python Documentation for the list "
"of available functions.\n\n");
+ fprintf(ofp, "from __future__ import print_function\n\n");
fprintf(ofp, "import os\n");
fprintf(ofp, "import sys\n\n");
@@ -1636,10 +1637,10 @@ static int python_generate_script(struct pevent
*pevent, const char *outfile)
fprintf(ofp, "from Core import *\n\n\n");
fprintf(ofp, "def trace_begin():\n");
- fprintf(ofp, "\tprint \"in trace_begin\"\n\n");
+ fprintf(ofp, "\tprint(\"in trace_begin\")\n\n");
fprintf(ofp, "def trace_end():\n");
- fprintf(ofp, "\tprint \"in trace_end\"\n\n");
+ fprintf(ofp, "\tprint(\"in trace_end\")\n\n");
while ((event = trace_find_next_event(pevent, event))) {
fprintf(ofp, "def %s__%s(", event->syste
[tip:perf/urgent] perf tools: Generate a Python script compatible with Python 2 and 3
Commit-ID: 877cc639686b68c7de179a485544f4761e376b30
Gitweb: https://git.kernel.org/tip/877cc639686b68c7de179a485544f4761e376b30
Author: Jeremy Cline
AuthorDate: Tue, 8 May 2018 21:27:43 +
Committer: Arnaldo Carvalho de Melo
CommitDate: Wed, 11 Jul 2018 10:01:50 -0300
perf tools: Generate a Python script compatible with Python 2 and 3
When generating a Python script with "perf script -g python", produce
one that is compatible with Python 2 and 3. The difference between the
two generated scripts is:
--- python2-perf-script.py2018-05-08 15:35:00.865889705 -0400
+++ python3-perf-script.py2018-05-08 15:34:49.019789564 -0400
@@ -7,6 +7,8 @@
# be retrieved using Python functions of the form common_*(context).
# See the perf-script-python Documentation for the list of available
functions.
+from __future__ import print_function
+
import os
import sys
@@ -18,10 +20,10 @@
def trace_begin():
- print "in trace_begin"
+ print("in trace_begin")
def trace_end():
- print "in trace_end"
+ print("in trace_end")
def raw_syscalls__sys_enter(event_name, context, common_cpu,
common_secs, common_nsecs, common_pid, common_comm,
@@ -29,26 +31,26 @@
print_header(event_name, common_cpu, common_secs, common_nsecs,
common_pid, common_comm)
- print "id=%d, args=%s" % \
- (id, args)
+ print("id=%d, args=%s" % \
+ (id, args))
- print 'Sample:
{'+get_dict_as_string(perf_sample_dict['sample'], ', ')+'}'
+ print('Sample:
{'+get_dict_as_string(perf_sample_dict['sample'], ', ')+'}')
for node in common_callchain:
if 'sym' in node:
- print "\t[%x] %s" % (node['ip'],
node['sym']['name'])
+ print("\t[%x] %s" % (node['ip'],
node['sym']['name']))
else:
- print " [%x]" % (node['ip'])
+ print(" [%x]" % (node['ip']))
- print "\n"
+ print()
def trace_unhandled(event_name, context, event_fields_dict,
perf_sample_dict):
- print get_dict_as_string(event_fields_dict)
- print 'Sample:
{'+get_dict_as_string(perf_sample_dict['sample'], ', ')+'}'
+ print(get_dict_as_string(event_fields_dict))
+ print('Sample:
{'+get_dict_as_string(perf_sample_dict['sample'], ', ')+'}')
def print_header(event_name, cpu, secs, nsecs, pid, comm):
- print "%-20s %5u %05u.%09u %8u %-20s " % \
- (event_name, cpu, secs, nsecs, pid, comm),
+ print("%-20s %5u %05u.%09u %8u %-20s " % \
+ (event_name, cpu, secs, nsecs, pid, comm), end="")
def get_dict_as_string(a_dict, delimiter=' '):
return delimiter.join(['%s=%s'%(k,str(v))for k,v in
sorted(a_dict.items())])
Signed-off-by: Jeremy Cline
Cc: Alexander Shishkin
Cc: Herton Krzesinski
Cc: Jiri Olsa
Cc: Namhyung Kim
Cc: Peter Zijlstra
Link:
http://lkml.kernel.org/r/0100016341a7278a-d178c724-2b0f-49ca-be93-80a7d51aaa0d-000...@email.amazonses.com
Signed-off-by: Arnaldo Carvalho de Melo
---
.../util/scripting-engines/trace-event-python.c| 29 +++---
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/tools/perf/util/scripting-engines/trace-event-python.c
b/tools/perf/util/scripting-engines/trace-event-python.c
index 46e9e19ab1ac..8b2eb6dbff4d 100644
--- a/tools/perf/util/scripting-engines/trace-event-python.c
+++ b/tools/perf/util/scripting-engines/trace-event-python.c
@@ -1627,6 +1627,7 @@ static int python_generate_script(struct pevent *pevent,
const char *outfile)
fprintf(ofp, "# See the perf-script-python Documentation for the list "
"of available functions.\n\n");
+ fprintf(ofp, "from __future__ import print_function\n\n");
fprintf(ofp, "import os\n");
fprintf(ofp, "import sys\n\n");
@@ -1636,10 +1637,10 @@ static int python_generate_script(struct pevent
*pevent, const char *outfile)
fprintf(ofp, "from Core import *\n\n\n");
fprintf(ofp, "def trace_begin():\n");
- fprintf(ofp, "\tprint \"in trace_begin\"\n\n");
+ fprintf(ofp, "\tprint(\"in trace_begin\")\n\n");
fprintf(ofp, "def trace_end():\n");
- fprintf(ofp, "\tprint \"in trace_end\"\n\n");
+ fprintf(ofp, "\tprint(\"in trace_end\")\n\n");
while ((event = trace_find_next_event(pevent, event))) {
fprintf(ofp, "def %s__%s(", event->syste
[tip:perf/urgent] perf scripts python: Add Python 3 support to SchedGui.py
Commit-ID: 2ab89262ff8895b8476b97345507c676fe3081fa
Gitweb: https://git.kernel.org/tip/2ab89262ff8895b8476b97345507c676fe3081fa
Author: Jeremy Cline
AuthorDate: Tue, 8 May 2018 21:27:45 +
Committer: Arnaldo Carvalho de Melo
CommitDate: Wed, 11 Jul 2018 10:01:50 -0300
perf scripts python: Add Python 3 support to SchedGui.py
Fix a single syntax error in SchedGui.py to support both Python 2 and
Python 3. This should have no functional change.
Signed-off-by: Jeremy Cline
Cc: Alexander Shishkin
Cc: Herton Krzesinski
Cc: Jiri Olsa
Cc: Namhyung Kim
Cc: Peter Zijlstra
Link:
http://lkml.kernel.org/r/0100016341a72d26-75729663-fe55-4309-8c9b-302e065ed2f1-000...@email.amazonses.com
Signed-off-by: Arnaldo Carvalho de Melo
---
tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/SchedGui.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git
a/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/SchedGui.py
b/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/SchedGui.py
index fdd92f699055..cac7b2542ee8 100644
--- a/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/SchedGui.py
+++ b/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/SchedGui.py
@@ -11,7 +11,7 @@
try:
import wx
except ImportError:
- raise ImportError, "You need to install the wxpython lib for this
script"
+ raise ImportError("You need to install the wxpython lib for this
script")
class RootFrame(wx.Frame):
[tip:perf/urgent] perf scripts python: Add Python 3 support to SchedGui.py
Commit-ID: 2ab89262ff8895b8476b97345507c676fe3081fa
Gitweb: https://git.kernel.org/tip/2ab89262ff8895b8476b97345507c676fe3081fa
Author: Jeremy Cline
AuthorDate: Tue, 8 May 2018 21:27:45 +
Committer: Arnaldo Carvalho de Melo
CommitDate: Wed, 11 Jul 2018 10:01:50 -0300
perf scripts python: Add Python 3 support to SchedGui.py
Fix a single syntax error in SchedGui.py to support both Python 2 and
Python 3. This should have no functional change.
Signed-off-by: Jeremy Cline
Cc: Alexander Shishkin
Cc: Herton Krzesinski
Cc: Jiri Olsa
Cc: Namhyung Kim
Cc: Peter Zijlstra
Link:
http://lkml.kernel.org/r/0100016341a72d26-75729663-fe55-4309-8c9b-302e065ed2f1-000...@email.amazonses.com
Signed-off-by: Arnaldo Carvalho de Melo
---
tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/SchedGui.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git
a/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/SchedGui.py
b/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/SchedGui.py
index fdd92f699055..cac7b2542ee8 100644
--- a/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/SchedGui.py
+++ b/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/SchedGui.py
@@ -11,7 +11,7 @@
try:
import wx
except ImportError:
- raise ImportError, "You need to install the wxpython lib for this
script"
+ raise ImportError("You need to install the wxpython lib for this
script")
class RootFrame(wx.Frame):
[tip:perf/urgent] perf scripts python: Add Python 3 support to Core.py
Commit-ID: 770d2f86c0051d4f2c0ab9d74d68434cb383241d Gitweb: https://git.kernel.org/tip/770d2f86c0051d4f2c0ab9d74d68434cb383241d Author: Jeremy Cline AuthorDate: Tue, 8 May 2018 21:27:45 + Committer: Arnaldo Carvalho de Melo CommitDate: Wed, 11 Jul 2018 10:01:50 -0300 perf scripts python: Add Python 3 support to Core.py Support both Python 2 and Python 3 in Core.py. This should have no functional change. Signed-off-by: Jeremy Cline Cc: Alexander Shishkin Cc: Herton Krzesinski Cc: Jiri Olsa Cc: Namhyung Kim Cc: Peter Zijlstra Link: http://lkml.kernel.org/r/0100016341a72ebe-e572899e-f445-4765-98f0-c314935727f9-000...@email.amazonses.com Signed-off-by: Arnaldo Carvalho de Melo --- .../python/Perf-Trace-Util/lib/Perf/Trace/Core.py | 40 +- 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/Core.py b/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/Core.py index 38dfb720fb6f..54ace2f6bc36 100644 --- a/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/Core.py +++ b/tools/perf/scripts/python/Perf-Trace-Util/lib/Perf/Trace/Core.py @@ -31,10 +31,8 @@ def flag_str(event_name, field_name, value): string = "" if flag_fields[event_name][field_name]: - print_delim = 0 -keys = flag_fields[event_name][field_name]['values'].keys() -keys.sort() -for idx in keys: +print_delim = 0 +for idx in sorted(flag_fields[event_name][field_name]['values']): if not value and not idx: string += flag_fields[event_name][field_name]['values'][idx] break @@ -51,14 +49,12 @@ def symbol_str(event_name, field_name, value): string = "" if symbolic_fields[event_name][field_name]: -keys = symbolic_fields[event_name][field_name]['values'].keys() -keys.sort() -for idx in keys: +for idx in sorted(symbolic_fields[event_name][field_name]['values']): if not value and not idx: - string = symbolic_fields[event_name][field_name]['values'][idx] +string = symbolic_fields[event_name][field_name]['values'][idx] break - if (value == idx): - string = symbolic_fields[event_name][field_name]['values'][idx] +if (value == idx): +string = symbolic_fields[event_name][field_name]['values'][idx] break return string @@ -74,19 +70,17 @@ def trace_flag_str(value): string = "" print_delim = 0 -keys = trace_flags.keys() - -for idx in keys: - if not value and not idx: - string += "NONE" - break - - if idx and (value & idx) == idx: - if print_delim: - string += " | "; - string += trace_flags[idx] - print_delim = 1 - value &= ~idx +for idx in trace_flags: +if not value and not idx: +string += "NONE" +break + +if idx and (value & idx) == idx: +if print_delim: +string += " | "; +string += trace_flags[idx] +print_delim = 1 +value &= ~idx return string
