[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 = &bios_regs; > > pool->base.funcs = &dcn20_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 spa
[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 = &bios_regs; pool->base.funcs = &dcn20_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, &nvbo); + 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(&drm->clients_lock); + list_for_each_entry_safe(cli, temp_cli, &drm->clients, head) { + list_del(&cli->head); + mutex_lock(&cli->mutex); + if (cli->abi16) + nouveau_abi16_fini(cli->abi16); + mutex_unlock(&cli->mutex); + nouveau_cli_fini(cli); + kfree(cli); + } + mutex_unlock(&drm->clients_lock); + nouveau_cli_fini(&drm->client); nouveau_cli_fini(&drm->master); nvif_parent_dtor(&drm->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, &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(&drm->clients); + mutex_init(&drm->clients_lock); spin_lock_init(&drm->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(&drm->client); nouveau_cli_fini(&drm->master); nvif_parent_dtor(&drm->parent); + mutex_destroy(&drm->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(&drm->client.mutex); + mutex_lock(&drm->clients_lock); list_add(&cli->head, &drm->clients); - mutex_unlock(&drm->client.mutex); + mutex_unlock(&drm->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(&cli->mutex); - mutex_lock(&drm->client.mutex); + mutex_lock(&drm->clients_lock); list_del(&cli->head); - mutex_unlock(&drm->client.mutex); + mutex_unlock(&drm->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 &struct 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(&drm->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(&drm->clients); > > + mutex_init(&drm->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(&drm->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(&drm->client.mutex); > > + mutex_lock(&drm->clients_lock); > > list_add(&cli->head, &drm->clients); > > - mutex_unlock(&drm->client.mutex); > > + mutex_unlock(&drm->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(&cli->mutex); > > > > - mutex_lock(&drm->client.mutex); > > + mutex_lock(&drm->clients_lock); > > list_del(&cli->head); > > - mutex_unlock(&drm->client.mutex); > > + mutex_unlock(&drm->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 &struct > > 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(&nouveau_parent, &drm->parent); > > drm->master.base.object.parent = &drm->parent; > > @@ -620,7 +617,6 @@ nouveau_drm_device_init(struct drm_device *dev) > > nouveau_cli_fini(&drm->master); > > fail_alloc: > > nvif_parent_dtor(&drm->parent); > > - kfree(drm); > > return ret; > > } > > > > @@ -654,7 +650,6 @@ nouveau_drm_device_fini(struct drm_device *dev) > > nouveau_cli_fini(&drm->client); > > nouveau_cli_fini(&drm->master); > > nvif_parent_dtor(&drm->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(&driver_pci, &pdev->dev); > > - if (IS_ERR(drm_dev)) { > > - ret = PTR_ERR(drm_dev); > > + nv_dev = devm_drm_dev_alloc(&pdev->dev, &driver_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,
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(&nouveau_parent, &drm->parent); > > drm->master.base.object.parent = &drm->parent; > > @@ -620,7 +617,6 @@ nouveau_drm_device_init(struct drm_device *dev) > > nouveau_cli_fini(&drm->master); > > fail_alloc: > > nvif_parent_dtor(&drm->parent); > > - kfree(drm); > > return ret; > > } > > > > @@ -654,7 +650,6 @@ nouveau_drm_device_fini(struct drm_device *dev) > > nouveau_cli_fini(&drm->client); > > nouveau_cli_fini(&drm->master); > > nvif_parent_dtor(&drm->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(&driver_pci, &pdev->dev); > > - if (IS_ERR(drm_dev)) { > > - ret = PTR_ERR(drm_dev); > > + nv_dev = devm_drm_dev_alloc(&pdev->dev, &driver_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(&device); > >
[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(&nouveau_parent, &drm->parent); drm->master.base.object.parent = &drm->parent; @@ -620,7 +617,6 @@ nouveau_drm_device_init(struct drm_device *dev) nouveau_cli_fini(&drm->master); fail_alloc: nvif_parent_dtor(&drm->parent); - kfree(drm); return ret; } @@ -654,7 +650,6 @@ nouveau_drm_device_fini(struct drm_device *dev) nouveau_cli_fini(&drm->client); nouveau_cli_fini(&drm->master); nvif_parent_dtor(&drm->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(&driver_pci, &pdev->dev); - if (IS_ERR(drm_dev)) { - ret = PTR_ERR(drm_dev); + nv_dev = devm_drm_dev_alloc(&pdev->dev, &driver_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(&device); 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(&device); } @@ -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(&driver_platform, &pdev->dev); - if (IS_ERR(drm)) { - err = PTR_ERR(drm); + nv_dev = devm_drm_dev_alloc(&pdev->dev, &driver_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
[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, &state)) { - 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)
[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 &struct 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 &struct +* nvif_mem embedded in &struct 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 &struct 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 &struct 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 &struct 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(&drm->clients); + mutex_init(&drm->clients_lock); spin_lock_init(&drm->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(&drm->client.mutex); + mutex_lock(&drm->clients_lock); list_add(&cli->head, &drm->clients); - mutex_unlock(&drm->client.mutex); + mutex_unlock(&drm->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(&cli->mutex); - mutex_lock(&drm->client.mutex); + mutex_lock(&drm->clients_lock); list_del(&cli->head); - mutex_unlock(&drm->client.mutex); + mutex_unlock(&drm->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 &struct 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(&drm->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(&drm->clients_lock); + list_for_each_entry_safe(cli, temp_cli, &drm->clients, head) { + list_del(&cli->head); + mutex_lock(&cli->mutex); + if (cli->abi16) + nouveau_abi16_fini(cli->abi16); + mutex_unlock(&cli->mutex); + nouveau_cli_fini(cli); + kfree(cli); + } + mutex_unlock(&drm->clients_lock); + nouveau_cli_fini(&drm->client); nouveau_cli_fini(&drm->master); nvif_parent_dtor(&drm->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, &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(&drm->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(&drm->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, &temp); > > > + *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(&drm->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(&drm->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, &temp); + *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++] = &temp1_auto_point_sensor_group; if (therm->fan_get && therm->fan_get(therm) >= 0) special_groups[i++] = &pwm_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 = &ignored_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)
[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, &temp); - *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/subde
[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 = &therm->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 <
[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, &lockdown_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, > > &lockdown_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, &hu->flags); > > err = hci_uart_register_dev(hu); > if (err) { > - clear_bit(HCI_UART_PROTO_READY, &hu->flags); > return err; > } > > + set_bit(HCI_UART_PROTO_READY, &hu->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, &hu->flags); + hu->proto->close(hu); hdev = hu->hdev; hu->hdev = NULL; hci_free_dev(hdev); - clear_bit(HCI_UART_PROTO_READY, &hu->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, &hu->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, &hu->flags); err = hci_uart_register_dev(hu); if (err) { clear_bit(HCI_UART_PROTO_READY, &hu->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 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
[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&m=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] 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(&gov_dbs_data_mutex); > + policy_dbs = policy->governor_data; > + if (!policy_dbs) > + goto out; > > mutex_lock(&policy_dbs->update_mutex); > cpufreq_policy_apply_limits(policy); > gov_update_sample_delay(policy_dbs, 0); > > mutex_unlock(&policy_dbs->update_mutex); > +out: > + mutex_unlock(&gov_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)(&wfc.work)){+.+.}, at: __flush_work+0x28c/0x320 but task is already holding lock: 84d461de (&policy_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 (&policy_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 (&policy->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 kthre
[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, &state); @@ -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
[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
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 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&m=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 >> > > >
[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] 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 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 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
[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
[PATCH] perf: 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 --- 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 -- 2.17.1
Re: [PATCH 4.4 128/268] scsi: sd: Keep disk read-only when re-reading partition
Hi Ben, On 06/13/2018 05:25 PM, Ben Hutchings wrote: > On Mon, 2018-05-28 at 12:01 +0200, Greg Kroah-Hartman wrote: >> 4.4-stable review patch. If anyone has any objections, please let me know. >> >> ------ >> >> From: Jeremy Cline >> >> [ Upstream commit 20bd1d026aacc5399464f8328f305985c493cde3 ] >> >> If the read-only flag is true on a SCSI disk, re-reading the partition >> table sets the flag back to false. >> >> To observe this bug, you can run: >> >> 1. blockdev --setro /dev/sda >> 2. blockdev --rereadpt /dev/sda >> 3. blockdev --getro /dev/sda >> >> This commit reads the disk's old state and combines it with the device >> disk-reported state rather than unconditionally marking it as RW. > > It seems to me that this change is likely to cause a regression: if a > SCSI device switches from read-only to read-write state then a > subsequent rescan won't automatically change the block device to read- > write state. The administrator will have to use the blockdev command > too. > > Even if this change in behaviour is acceptable, this commit does not > implement it consistently. The function starts by clearing the ro flag > and this commit only changes one of the three exit paths to preserve > it. (The log message about Write Protect status also reports the > underlying SCSI device flag and not the combined ro flag, but maybe > that was intentional.) Yes, it looks like I messed this up. > > I think this commit should be reverted, both in stable and upstream. A > proper fix would involve splitting the ro flag into two flags—one > controlled by user-space and one read from the device—with the > effective read-only status being the logical-or of those two. This seems sensible to me, for whatever that's worth. I'm new to the kernel so I'm not certain I'll produce a reasonable patch, but I'm willing to give it a shot. So the change would be something like: 1. Add a flag to the gendisk struct to indicate if the device is read-only. The driver sets this. 2. In hd_struct struct's policy flag is one that indicates the user-space setting and this gets set by BLKROSET ioctl/set_disk_ro. 3. The effective read-only status reported by the BLKROGET ioctl and bdev_read_only is the logical-or of the hd_struct policy flag and the gendisk device flag. Maybe that doesn't make sense at all, though. Thoughts? Thanks, Jeremy
Re: Regression: x86/tsc: Fix mark_tsc_unstable()
On 06/11/2018 03:23 PM, Diego Viola wrote: > On Mon, Jun 11, 2018 at 3:11 PM, Jeremy Cline wrote: >> On 06/11/2018 01:56 PM, Jeremy Cline wrote: >>> On 06/11/2018 11:30 AM, Peter Zijlstra wrote: >>>> On Mon, Jun 11, 2018 at 04:38:01PM +0200, Peter Zijlstra wrote: >>>>> On Mon, Jun 11, 2018 at 04:17:42PM +0200, Peter Zijlstra wrote: >>>>>> On Mon, Jun 11, 2018 at 01:59:15PM +, Jeremy Cline wrote: >>>>>>> A user has bisected the problem to the v4.16 commit 1ab4ca7c59d4 >>>>>>> ("x86/tsc: Fix mark_tsc_unstable()"). According to the reporter, >>>>>>> explicitly setting "tsc=" on the kernel command line causes the boot to >>>>>>> always succeed. All the users have Thinkpad T500s or T400s (Core 2 Duos) >>>>>> >>>>>> Weird. So Core2 typically triggers mark_tsc_unstable() in either >>>>>> intel_idle or processor_idle. ISTR testing that when I did the patches. >>>>>> >>>>>> When I make that mark_tsc_unstable() in the idle drivers unconditional >>>>>> and boot my ivb with that, it doesn't want to fail. I've booted the >>>>>> machine 5 consequctive times without issue. >>>>>> >>>>>> Let me try and checkout -stable, maybe something's up with that. >>>>> >>>>> Nope -stable seems to be working as well on the IVB (with modification). >>>>> I just dug up my T500 and that's actually still running the test kernel. >>>>> Let me try and build the -stable kernel for that. >>>> >>>> 4.16.8 works without issue on my T500 with a debian/ubuntu like distro >>>> config. >>>> >>> >>> Adding mmarget (who bisected the problem) to the CC. >>> >>> It might well be something Fedora-specific, then. I just noticed mmarget >>> commented over the weekend noting that they couldn't reproduce the >>> problem without using the initramfs generated during the RPM install of >>> the kernel. mmarget's theory was that it's a race condition that doesn't >>> occur when the initramfs takes long enough to unpack, but I don't know >>> enough about the early boot process *or* how Fedora's generating the >>> initramfs for RPM installs vs "make install" yet to know how likely that >>> is. I'm going to have to do some research. >>> >>> Thanks for looking into this so quickly and also sorry if this turns out >>> to be a Fedora problem :( >> >> Attached is the Fedora configuration for 4.16.8, as well, in case you'd >> like to test it with that. >> >> Thanks, >> Jeremy > > Hi Jeremy, > > I've compiled 4.16.8 with your config and booted my machine about 10 > times with this kernel, and I'm unable to reproduce the issue. Thanks for confirming. > > Maybe it's an issue with the Fedora initramfs? Indeed, I'll dig into what exactly is different about the RPM-created initramfs and the one created with "make install" to see if we can narrow this down some more. Thanks, Jeremy
Re: Regression: x86/tsc: Fix mark_tsc_unstable()
On 06/11/2018 11:30 AM, Peter Zijlstra wrote: > On Mon, Jun 11, 2018 at 04:38:01PM +0200, Peter Zijlstra wrote: >> On Mon, Jun 11, 2018 at 04:17:42PM +0200, Peter Zijlstra wrote: >>> On Mon, Jun 11, 2018 at 01:59:15PM +, Jeremy Cline wrote: >>>> A user has bisected the problem to the v4.16 commit 1ab4ca7c59d4 >>>> ("x86/tsc: Fix mark_tsc_unstable()"). According to the reporter, >>>> explicitly setting "tsc=" on the kernel command line causes the boot to >>>> always succeed. All the users have Thinkpad T500s or T400s (Core 2 Duos) >>> >>> Weird. So Core2 typically triggers mark_tsc_unstable() in either >>> intel_idle or processor_idle. ISTR testing that when I did the patches. >>> >>> When I make that mark_tsc_unstable() in the idle drivers unconditional >>> and boot my ivb with that, it doesn't want to fail. I've booted the >>> machine 5 consequctive times without issue. >>> >>> Let me try and checkout -stable, maybe something's up with that. >> >> Nope -stable seems to be working as well on the IVB (with modification). >> I just dug up my T500 and that's actually still running the test kernel. >> Let me try and build the -stable kernel for that. > > 4.16.8 works without issue on my T500 with a debian/ubuntu like distro > config. > Adding mmarget (who bisected the problem) to the CC. It might well be something Fedora-specific, then. I just noticed mmarget commented over the weekend noting that they couldn't reproduce the problem without using the initramfs generated during the RPM install of the kernel. mmarget's theory was that it's a race condition that doesn't occur when the initramfs takes long enough to unpack, but I don't know enough about the early boot process *or* how Fedora's generating the initramfs for RPM installs vs "make install" yet to know how likely that is. I'm going to have to do some research. Thanks for looking into this so quickly and also sorry if this turns out to be a Fedora problem :( Thanks, Jeremy
Regression: x86/tsc: Fix mark_tsc_unstable()
Hi folks, A few Fedora users have reported[0] a regression starting in v4.16.8 where the boot will hang ~1/3 of the time with the following RCU stall warning: INFO: rcu_sched detected stalls on CPUs/tasks: o1-...!: (0 ticks this GP) idle=688/0/0 softirq=171/171 fqs=0 o(detected by 0, t=60002 jiffies, g=-142, c=-143, q=9) Sending NMI from CPU 0 to CPU 1: NMI backtrace for cpu 1 skipped: idling at acpi_processor_ffh_cstate_enter+0x65/0xb0 rcu_sched kthread starved for 60002 jiffies! g18446744073709551474 c1844674407370955143 f0x0 RCU_GP_WAIT_FQS(3) ->state=0x402 -> cpu=1 RCU grace-period kthread stack dump: rcu_sched I0 9 2 0x8000 Call Trace: ? __schedule+0x234/0x850 schedule+0x28/0x80 schedule_timeout+0x166/0x380 ? __next_timer_interrupt+0xc0/0xc0 rcu_gp_kthread+0x368/0x830 ? rcu_process_callbacks+0x4f0/0x4f0 kthread+0x112/0x130 ? kthread_create_worker_on_cpu+0x70/0x70 ret_from_fork+0x35/0x40 A user has bisected the problem to the v4.16 commit 1ab4ca7c59d4 ("x86/tsc: Fix mark_tsc_unstable()"). According to the reporter, explicitly setting "tsc=" on the kernel command line causes the boot to always succeed. All the users have Thinkpad T500s or T400s (Core 2 Duos) [0] https://bugzilla.redhat.com/show_bug.cgi?id=1579925 Thanks, Jeremy
[PATCH 1/8] 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 --- .../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 10dd5fce082b..01d5f255ef62 100644 --- a/tools/perf/util/scripting-engines/trace-event-python.c +++ b/tools/perf/util/scripting-engines/trace-event-python.c @@ -1393,6 +1393,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"); @@ -1402,10 +1403,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->system, event->name); @@ -1441,7 +1442,7 @@ static int python_generate_script(struct pevent *pevent, const char *outfile) "common_secs, common_nsecs,\n\t\t\t" "common_pid, common_comm)\n\n"); - fprintf(ofp, "\t\tprint \""); + fprintf(o
[PATCH 3/8] 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 --- .../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): -- 2.17.0
[PATCH 2/8] 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 --- .../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 -- 2.17.0
[PATCH 4/8] 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 --- .../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: -- 2.17.0
[PATCH 8/8] perf tests: Add Python 3 support to attr.py
Support both Python 2 and Python 3 in attr.py. This should have no functional change. Signed-off-by: Jeremy Cline --- tools/perf/tests/attr.py | 32 +++- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/tools/perf/tests/attr.py b/tools/perf/tests/attr.py index ff9b60b99f52..15fbf38450fe 100644 --- a/tools/perf/tests/attr.py +++ b/tools/perf/tests/attr.py @@ -1,6 +1,8 @@ #! /usr/bin/python # SPDX-License-Identifier: GPL-2.0 +from __future__ import print_function + import os import sys import glob @@ -8,7 +10,11 @@ import optparse import tempfile import logging import shutil -import ConfigParser +try: +import ConfigParser as configparser +except ImportError: +# Python 3 renamed ConfigParser to configparser +import configparser def data_equal(a, b): # Allow multiple values in assignment separated by '|' @@ -100,23 +106,23 @@ class Event(dict): def equal(self, other): for t in Event.terms: log.debug(" [%s] %s %s" % (t, self[t], other[t])); -if not self.has_key(t) or not other.has_key(t): +if t not in self or t not in other: return False if not data_equal(self[t], other[t]): return False return True def optional(self): -if self.has_key('optional') and self['optional'] == '1': +if 'optional' in self and self['optional'] == '1': return True return False def diff(self, other): for t in Event.terms: -if not self.has_key(t) or not other.has_key(t): +if t not in self or t not in other: continue if not data_equal(self[t], other[t]): - log.warning("expected %s=%s, got %s" % (t, self[t], other[t])) +log.warning("expected %s=%s, got %s" % (t, self[t], other[t])) # Test file description needs to have following sections: # [config] @@ -134,7 +140,7 @@ class Event(dict): # - expected values assignments class Test(object): def __init__(self, path, options): -parser = ConfigParser.SafeConfigParser() +parser = configparser.SafeConfigParser() parser.read(path) log.warning("running '%s'" % path) @@ -193,7 +199,7 @@ class Test(object): return True def load_events(self, path, events): -parser_event = ConfigParser.SafeConfigParser() +parser_event = configparser.SafeConfigParser() parser_event.read(path) # The event record section header contains 'event' word, @@ -207,7 +213,7 @@ class Test(object): # Read parent event if there's any if (':' in section): base = section[section.index(':') + 1:] -parser_base = ConfigParser.SafeConfigParser() +parser_base = configparser.SafeConfigParser() parser_base.read(self.test_dir + '/' + base) base_items = parser_base.items('event') @@ -322,9 +328,9 @@ def run_tests(options): for f in glob.glob(options.test_dir + '/' + options.test): try: Test(f, options).run() -except Unsup, obj: +except Unsup as obj: log.warning("unsupp %s" % obj.getMsg()) -except Notest, obj: +except Notest as obj: log.warning("skipped %s" % obj.getMsg()) def setup_log(verbose): @@ -373,7 +379,7 @@ def main(): setup_log(options.verbose) if not options.test_dir: -print 'FAILED no -d option specified' +print('FAILED no -d option specified') sys.exit(-1) if not options.test: @@ -382,8 +388,8 @@ def main(): try: run_tests(options) -except Fail, obj: -print "FAILED %s" % obj.getMsg(); +except Fail as obj: +print("FAILED %s" % obj.getMsg()) sys.exit(-1) sys.exit(0) -- 2.17.0
[PATCH 7/8] perf scripts python: Add Python 3 support to stat-cpi.py
Support both Python 2 and Python 3 in stat-cpi.py. This should have no functional change. Signed-off-by: Jeremy Cline --- tools/perf/scripts/python/stat-cpi.py | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tools/perf/scripts/python/stat-cpi.py b/tools/perf/scripts/python/stat-cpi.py index 8410672efb8b..fc53a03da412 100644 --- a/tools/perf/scripts/python/stat-cpi.py +++ b/tools/perf/scripts/python/stat-cpi.py @@ -1,6 +1,8 @@ #!/usr/bin/env python # SPDX-License-Identifier: GPL-2.0 +from __future__ import print_function + data= {} times = [] threads = [] @@ -59,7 +61,8 @@ def stat__interval(time): if ins != 0: cpi = cyc/float(ins) -print "%15f: cpu %d, thread %d -> cpi %f (%d/%d)" % (time/(float(10)), cpu, thread, cpi, cyc, ins) +print("%15f: cpu %d, thread %d -> cpi %f (%d/%d)" % + (time/(float(10)), cpu, thread, cpi, cyc, ins)) def trace_end(): pass -- 2.17.0
[PATCH 5/8] 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 --- .../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 -- 2.17.0
[PATCH 6/8] 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 --- 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 -- 2.17.0
[PATCH 0/8] Improve Python 3 support in perf
Hi folks, This patch set adds Python 3 support to a number of perf scripts, including those generated by "perf script -g python", without breaking Python 2 compatibility. Jeremy Cline (8): perf tools: Generate a Python script compatible with Python 2 and 3 perf scripts python: Add Python 3 support to Core.py perf scripts python: Add Python 3 support to SchedGui.py perf scripts python: Add Python 3 support to Util.py perf scripts python: Add Python 3 support to EventClass.py perf scripts python: Add Python 3 support to sched-migration.py perf scripts python: Add Python 3 support to stat-cpi.py perf tests: Add Python 3 support to attr.py .../Perf-Trace-Util/lib/Perf/Trace/Core.py| 40 --- .../lib/Perf/Trace/EventClass.py | 4 +- .../lib/Perf/Trace/SchedGui.py| 2 +- .../Perf-Trace-Util/lib/Perf/Trace/Util.py| 11 ++--- tools/perf/scripts/python/sched-migration.py | 14 --- tools/perf/scripts/python/stat-cpi.py | 5 ++- tools/perf/tests/attr.py | 32 +-- .../scripting-engines/trace-event-python.c| 29 +++--- 8 files changed, 74 insertions(+), 63 deletions(-) -- 2.17.0
Re: Linux messages full of `random: get_random_u32 called from`
On 04/29/2018 06:05 PM, Theodore Y. Ts'o wrote: > On Sun, Apr 29, 2018 at 01:20:33PM -0700, Sultan Alsawaf wrote: >> On Sun, Apr 29, 2018 at 08:41:01PM +0200, Pavel Machek wrote: >>> Umm. No. https://www.youtube.com/watch?v=xneBjc8z0DE >> >> Okay, but /dev/urandom isn't a solution to this problem because it isn't >> usable >> until crng init is complete, so it suffers from the same init lag as >> /dev/random. > > It's more accurate to say that using /dev/urandom is no worse than > before (from a few years ago). There are, alas, plenty of > distributions and user space application programmers that basically > got lazy using /dev/urandom, and assumed that there would be plenty of > entropy during early system startup. > > When they switched over the getrandom(2), the most egregious examples > of this caused pain (and they got fixed), but due to a bug in > drivers/char/random.c, if getrandom(2) was called after the entropy > pool was "half initialized", it would not block, but proceed. > > Is that exploitable? Well, Jann and I didn't find an _obvious_ way to > exploit the short coming, which is this wasn't treated like an > emergency situation ala the embarassing situation we had five years > ago[1]. > > [1] https://factorable.net/paper.html > > However, it was enough to make us be uncomfortable, which is why I > pushed the changes that I did. At least on the devices we had at > hand, using the distributions that we typically use, the impact seemed > minimal. Unfortuantely, there is no way to know for sure without > rolling out change and seeing who screams. In the ideal world, > software would not require cryptographic randomness immediately after > boot, before the user logs in. And ***really***, as in [1], softwaret > should not be generating long-term public keys that are essential to > the security of the box a few seconds immediately after the device is > first unboxed and plugged in.i > > What would be useful is if people gave reports that listed exactly > what laptop and distributions they are using. Just "a high spec x86 > laptop" isn't terribly useful, because *my* brand-new Dell XPS 13 > running Debian testing is working just fine. The year, model, make, > and CPU type plus what distribution (and distro version number) you > are running is useful, so I can assess how wide spread the unhappiness > is going to be, and what mitigation steps make sense. Fedora has started seeing some bug reports on this for Fedora 27[0] and I've asked reporters to include their hardware details. [0] https://bugzilla.redhat.com/show_bug.cgi?id=1572944 Regards, Jeremy
Re: Regression from efi: call get_event_log before ExitBootServices
On 03/13/2018 03:59 AM, Ard Biesheuvel wrote: > On 13 March 2018 at 07:47, Hans de Goede wrote: >> Hi, >> >> >> On 12-03-18 20:55, Thiebaud Weksteen wrote: >>> > ... >>> >>> Hans, you said you configured the tablet to use the 32-bit version of grub >>> instead >>> of 64. Why's that? >> >> >> Because this tablet, like (almost?) all Bay Trail hardware has a 32 bit >> UEFI, >> even though the processor is 64 bit capable (AFAIK 64 bit Windows drivers >> were >> not ready in time so all Bay Trail devices shipped with a 32 bit Windows). >> >> So this is running a 32 bit grub which boots a 64 bit kernel. >> >>> Jeremy, could you confirm if you are building the kernel in 64bit mode? Is >>> your Android install working? (that is, what happens if you boot >>> Boot)? >> >> >> AFAIK the kernel on Jeremy's tablet (which I initially installed) is 64 bit. >> >> Could the problem perhaps be that the new code for the TPM event-log is >> missing some handling to deal with running on a 32 bit firmware? I know the >> rest of the kernel has special code to deal with this. >> > > That is a very good point, and I missed that this is a 64-bit kernel > running on 32-bit UEFI. > > The TPM code does use efi_call_proto() directly, and now I am thinking > it is perhaps the allocate_pages() call that simply only initializes > the low 32-bits of log_tbl. > > Jeremy, could you please try initializing tcg2_protocol and log_tbl to > NULL at the start of the function? > That was it, it boots when those are initialized to NULL. Regards, Jeremy
Re: Regression from efi: call get_event_log before ExitBootServices
On 03/12/2018 03:55 PM, Thiebaud Weksteen wrote: > On Mon, Mar 12, 2018 at 7:33 PM Jeremy Cline wrote: > >> On 03/12/2018 02:29 PM, Thiebaud Weksteen wrote: >>> On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel < > ard.biesheu...@linaro.org> >>> wrote: >>> >>>> On 12 March 2018 at 17:01, Jeremy Cline wrote: >>>>> On 03/12/2018 10:56 AM, Ard Biesheuvel wrote: >>>>>> On 12 March 2018 at 14:30, Jeremy Cline wrote: >>>>>>> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote: >>>>>>>> On 10 March 2018 at 10:45, Thiebaud Weksteen >>> wrote: >>>>>>>>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline >>> wrote: >>>>>>>>> >>>>>>>>>> On Fri, Mar 09, 2018 at 10:43:50AM +, Thiebaud Weksteen > wrote: >>>>>>>>>>> Thanks a lot for trying out the patch! >>>>>>>>>>> >>>>>>>>>>> Please don't modify your install at this stage, I think we are >>> hitting a >>>>>>>>>>> firmware bug and that would be awesome if we can fix how we are >>>>>>>>> handling it. >>>>>>>>>>> So, if we reach that stage in the function it could either be >>> that: >>>>>>>>>>> * The allocation did not succeed, somehow, but the firmware > still >>>>>>>>> returned >>>>>>>>>>> EFI_SUCCEED. >>>>>>>>>>> * The size requested is incorrect (I'm thinking something like a >>> 1G of >>>>>>>>>>> log). This would be due to either a miscalculation of log_size >>>>>>>>> (possible) >>>>>>>>>>> or; the returned values of GetEventLog are not correct. >>>>>>>>>>> I'm sending a patch to add checks for these. Could you please >>> apply and >>>>>>>>>>> retest? >>>>>>>>>>> Again, thanks for helping debugging this. >>>>>>>>> >>>>>>>>>> No problem, thanks for the help :) >>>>>>>>> >>>>>>>>>> With the new patch: >>>>>>>>> >>>>>>>>>> Locating the TCG2Protocol >>>>>>>>>> Calling GetEventLog on TCG2Protocol >>>>>>>>>> Log returned >>>>>>>>>> log_location is not empty >>>>>>>>>> log_size != 0 >>>>>>>>>> log_size < 1M >>>>>>>>>> Allocating memory for storing the logs >>>>>>>>>> Returned from memory allocation >>>>>>>>>> Copying log to new location >>>>>>>>> >>>>>>>>>> And then it hangs. I added a couple more print statements: >>>>>>>>> >>>>>>>>>> diff --git a/drivers/firmware/efi/libstub/tpm.c >>>>>>>>> b/drivers/firmware/efi/libstub/tpm.c >>>>>>>>>> index ee3fac109078..1ab5638bc50e 100644 >>>>>>>>>> --- a/drivers/firmware/efi/libstub/tpm.c >>>>>>>>>> +++ b/drivers/firmware/efi/libstub/tpm.c >>>>>>>>>> @@ -148,8 +148,11 @@ void >>>>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) >>>>>>>>>> efi_printk(sys_table_arg, "Copying log to new >>> location\n"); >>>>>>>>> >>>>>>>>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size); >>>>>>>>>> + efi_printk(sys_table_arg, "Successfully memset log_tbl to >>> 0\n"); >>>>>>>>>> log_tbl->size = log_size; >>>>>>>>>> + efi_printk(sys_table_arg, "Set log_tbl->size\n"); >>>>>>>>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2; >>>>>>>>>> + efi_printk(sys_table_arg, "Set log_tbl-version\n"); >>>>>>>>>> memcpy(log_tbl->log, (void *) first_entry_addr, > log_size); >>>>>>>>> >>>>>>>>>>
Re: Regression from efi: call get_event_log before ExitBootServices
On 03/12/2018 02:29 PM, Thiebaud Weksteen wrote: > On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel > wrote: > >> On 12 March 2018 at 17:01, Jeremy Cline wrote: >>> On 03/12/2018 10:56 AM, Ard Biesheuvel wrote: >>>> On 12 March 2018 at 14:30, Jeremy Cline wrote: >>>>> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote: >>>>>> On 10 March 2018 at 10:45, Thiebaud Weksteen > wrote: >>>>>>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline > wrote: >>>>>>> >>>>>>>> On Fri, Mar 09, 2018 at 10:43:50AM +, Thiebaud Weksteen wrote: >>>>>>>>> Thanks a lot for trying out the patch! >>>>>>>>> >>>>>>>>> Please don't modify your install at this stage, I think we are > hitting a >>>>>>>>> firmware bug and that would be awesome if we can fix how we are >>>>>>> handling it. >>>>>>>>> So, if we reach that stage in the function it could either be > that: >>>>>>>>> * The allocation did not succeed, somehow, but the firmware still >>>>>>> returned >>>>>>>>> EFI_SUCCEED. >>>>>>>>> * The size requested is incorrect (I'm thinking something like a > 1G of >>>>>>>>> log). This would be due to either a miscalculation of log_size >>>>>>> (possible) >>>>>>>>> or; the returned values of GetEventLog are not correct. >>>>>>>>> I'm sending a patch to add checks for these. Could you please > apply and >>>>>>>>> retest? >>>>>>>>> Again, thanks for helping debugging this. >>>>>>> >>>>>>>> No problem, thanks for the help :) >>>>>>> >>>>>>>> With the new patch: >>>>>>> >>>>>>>> Locating the TCG2Protocol >>>>>>>> Calling GetEventLog on TCG2Protocol >>>>>>>> Log returned >>>>>>>> log_location is not empty >>>>>>>> log_size != 0 >>>>>>>> log_size < 1M >>>>>>>> Allocating memory for storing the logs >>>>>>>> Returned from memory allocation >>>>>>>> Copying log to new location >>>>>>> >>>>>>>> And then it hangs. I added a couple more print statements: >>>>>>> >>>>>>>> diff --git a/drivers/firmware/efi/libstub/tpm.c >>>>>>> b/drivers/firmware/efi/libstub/tpm.c >>>>>>>> index ee3fac109078..1ab5638bc50e 100644 >>>>>>>> --- a/drivers/firmware/efi/libstub/tpm.c >>>>>>>> +++ b/drivers/firmware/efi/libstub/tpm.c >>>>>>>> @@ -148,8 +148,11 @@ void >>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) >>>>>>>> efi_printk(sys_table_arg, "Copying log to new > location\n"); >>>>>>> >>>>>>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size); >>>>>>>> + efi_printk(sys_table_arg, "Successfully memset log_tbl to > 0\n"); >>>>>>>> log_tbl->size = log_size; >>>>>>>> + efi_printk(sys_table_arg, "Set log_tbl->size\n"); >>>>>>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2; >>>>>>>> + efi_printk(sys_table_arg, "Set log_tbl-version\n"); >>>>>>>> memcpy(log_tbl->log, (void *) first_entry_addr, log_size); >>>>>>> >>>>>>>> efi_printk(sys_table_arg, "Installing the log into the >>>>>>> configuration table\n"); >>>>>>> >>>>>>>> and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) + > log_size);" >>>>>>> >>>>>>> Thanks. Well, it looks like the memory that is supposedly allocated > is not >>>>>>> usable. I'm thinking this is a firmware bug. >>>>>>> Ard, would you agree on this assumption? Thoughts on how to proceed? >>>>>>> >>>>>> >>>>>> I am rather puzzled why the allocate_pool() should succeed and
Re: Regression from efi: call get_event_log before ExitBootServices
On 03/12/2018 01:30 PM, Ard Biesheuvel wrote: > On 12 March 2018 at 17:01, Jeremy Cline wrote: >> On 03/12/2018 10:56 AM, Ard Biesheuvel wrote: >>> On 12 March 2018 at 14:30, Jeremy Cline wrote: >>>> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote: >>>>> On 10 March 2018 at 10:45, Thiebaud Weksteen wrote: >>>>>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline wrote: >>>>>> >>>>>>> On Fri, Mar 09, 2018 at 10:43:50AM +, Thiebaud Weksteen wrote: >>>>>>>> Thanks a lot for trying out the patch! >>>>>>>> >>>>>>>> Please don't modify your install at this stage, I think we are hitting >>>>>>>> a >>>>>>>> firmware bug and that would be awesome if we can fix how we are >>>>>> handling it. >>>>>>>> So, if we reach that stage in the function it could either be that: >>>>>>>> * The allocation did not succeed, somehow, but the firmware still >>>>>> returned >>>>>>>> EFI_SUCCEED. >>>>>>>> * The size requested is incorrect (I'm thinking something like a 1G of >>>>>>>> log). This would be due to either a miscalculation of log_size >>>>>> (possible) >>>>>>>> or; the returned values of GetEventLog are not correct. >>>>>>>> I'm sending a patch to add checks for these. Could you please apply and >>>>>>>> retest? >>>>>>>> Again, thanks for helping debugging this. >>>>>> >>>>>>> No problem, thanks for the help :) >>>>>> >>>>>>> With the new patch: >>>>>> >>>>>>> Locating the TCG2Protocol >>>>>>> Calling GetEventLog on TCG2Protocol >>>>>>> Log returned >>>>>>> log_location is not empty >>>>>>> log_size != 0 >>>>>>> log_size < 1M >>>>>>> Allocating memory for storing the logs >>>>>>> Returned from memory allocation >>>>>>> Copying log to new location >>>>>> >>>>>>> And then it hangs. I added a couple more print statements: >>>>>> >>>>>>> diff --git a/drivers/firmware/efi/libstub/tpm.c >>>>>> b/drivers/firmware/efi/libstub/tpm.c >>>>>>> index ee3fac109078..1ab5638bc50e 100644 >>>>>>> --- a/drivers/firmware/efi/libstub/tpm.c >>>>>>> +++ b/drivers/firmware/efi/libstub/tpm.c >>>>>>> @@ -148,8 +148,11 @@ void >>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) >>>>>>> efi_printk(sys_table_arg, "Copying log to new location\n"); >>>>>> >>>>>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size); >>>>>>> + efi_printk(sys_table_arg, "Successfully memset log_tbl to 0\n"); >>>>>>> log_tbl->size = log_size; >>>>>>> + efi_printk(sys_table_arg, "Set log_tbl->size\n"); >>>>>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2; >>>>>>> + efi_printk(sys_table_arg, "Set log_tbl-version\n"); >>>>>>> memcpy(log_tbl->log, (void *) first_entry_addr, log_size); >>>>>> >>>>>>> efi_printk(sys_table_arg, "Installing the log into the >>>>>> configuration table\n"); >>>>>> >>>>>>> and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) + log_size);" >>>>>> >>>>>> Thanks. Well, it looks like the memory that is supposedly allocated is >>>>>> not >>>>>> usable. I'm thinking this is a firmware bug. >>>>>> Ard, would you agree on this assumption? Thoughts on how to proceed? >>>>>> >>>>> >>>>> I am rather puzzled why the allocate_pool() should succeed and the >>>>> subsequent memset() should fail. This does not look like an issue that >>>>> is intimately related to TPM2 support, rather an issue in the firmware >>>>> that happens to get tickled after the change. >>>>> >>>>> Would you mind
Re: Regression from efi: call get_event_log before ExitBootServices
On 03/12/2018 10:56 AM, Ard Biesheuvel wrote: > On 12 March 2018 at 14:30, Jeremy Cline wrote: >> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote: >>> On 10 March 2018 at 10:45, Thiebaud Weksteen wrote: >>>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline wrote: >>>> >>>>> On Fri, Mar 09, 2018 at 10:43:50AM +, Thiebaud Weksteen wrote: >>>>>> Thanks a lot for trying out the patch! >>>>>> >>>>>> Please don't modify your install at this stage, I think we are hitting a >>>>>> firmware bug and that would be awesome if we can fix how we are >>>> handling it. >>>>>> So, if we reach that stage in the function it could either be that: >>>>>> * The allocation did not succeed, somehow, but the firmware still >>>> returned >>>>>> EFI_SUCCEED. >>>>>> * The size requested is incorrect (I'm thinking something like a 1G of >>>>>> log). This would be due to either a miscalculation of log_size >>>> (possible) >>>>>> or; the returned values of GetEventLog are not correct. >>>>>> I'm sending a patch to add checks for these. Could you please apply and >>>>>> retest? >>>>>> Again, thanks for helping debugging this. >>>> >>>>> No problem, thanks for the help :) >>>> >>>>> With the new patch: >>>> >>>>> Locating the TCG2Protocol >>>>> Calling GetEventLog on TCG2Protocol >>>>> Log returned >>>>> log_location is not empty >>>>> log_size != 0 >>>>> log_size < 1M >>>>> Allocating memory for storing the logs >>>>> Returned from memory allocation >>>>> Copying log to new location >>>> >>>>> And then it hangs. I added a couple more print statements: >>>> >>>>> diff --git a/drivers/firmware/efi/libstub/tpm.c >>>> b/drivers/firmware/efi/libstub/tpm.c >>>>> index ee3fac109078..1ab5638bc50e 100644 >>>>> --- a/drivers/firmware/efi/libstub/tpm.c >>>>> +++ b/drivers/firmware/efi/libstub/tpm.c >>>>> @@ -148,8 +148,11 @@ void >>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) >>>>> efi_printk(sys_table_arg, "Copying log to new location\n"); >>>> >>>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size); >>>>> + efi_printk(sys_table_arg, "Successfully memset log_tbl to 0\n"); >>>>> log_tbl->size = log_size; >>>>> + efi_printk(sys_table_arg, "Set log_tbl->size\n"); >>>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2; >>>>> + efi_printk(sys_table_arg, "Set log_tbl-version\n"); >>>>> memcpy(log_tbl->log, (void *) first_entry_addr, log_size); >>>> >>>>> efi_printk(sys_table_arg, "Installing the log into the >>>> configuration table\n"); >>>> >>>>> and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) + log_size);" >>>> >>>> Thanks. Well, it looks like the memory that is supposedly allocated is not >>>> usable. I'm thinking this is a firmware bug. >>>> Ard, would you agree on this assumption? Thoughts on how to proceed? >>>> >>> >>> I am rather puzzled why the allocate_pool() should succeed and the >>> subsequent memset() should fail. This does not look like an issue that >>> is intimately related to TPM2 support, rather an issue in the firmware >>> that happens to get tickled after the change. >>> >>> Would you mind trying replacing EFI_LOADER_DATA with >>> EFI_BOOT_SERVICES_DATA in the allocate_pool() call? >> >> Replacing EFI_LOADER_DATA with EFI_BOOT_SERVICES_DATA still hangs at the >> memset() call. >> > > Could you try the following please? (attached as well in case gmail mangles > it) > > diff --git a/drivers/firmware/efi/libstub/tpm.c > b/drivers/firmware/efi/libstub/tpm.c > index 2298560cea72..30d960a344b7 100644 > --- a/drivers/firmware/efi/libstub/tpm.c > +++ b/drivers/firmware/efi/libstub/tpm.c > @@ -70,6 +70,8 @@ void > efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) > size_t log_size, last_entry_size; > efi_bool_t truncated; > void *tcg2_protocol; &g
Re: Regression from efi: call get_event_log before ExitBootServices
On 03/12/2018 07:08 AM, Ard Biesheuvel wrote: > On 10 March 2018 at 10:45, Thiebaud Weksteen wrote: >> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline wrote: >> >>> On Fri, Mar 09, 2018 at 10:43:50AM +, Thiebaud Weksteen wrote: >>>> Thanks a lot for trying out the patch! >>>> >>>> Please don't modify your install at this stage, I think we are hitting a >>>> firmware bug and that would be awesome if we can fix how we are >> handling it. >>>> So, if we reach that stage in the function it could either be that: >>>> * The allocation did not succeed, somehow, but the firmware still >> returned >>>> EFI_SUCCEED. >>>> * The size requested is incorrect (I'm thinking something like a 1G of >>>> log). This would be due to either a miscalculation of log_size >> (possible) >>>> or; the returned values of GetEventLog are not correct. >>>> I'm sending a patch to add checks for these. Could you please apply and >>>> retest? >>>> Again, thanks for helping debugging this. >> >>> No problem, thanks for the help :) >> >>> With the new patch: >> >>> Locating the TCG2Protocol >>> Calling GetEventLog on TCG2Protocol >>> Log returned >>> log_location is not empty >>> log_size != 0 >>> log_size < 1M >>> Allocating memory for storing the logs >>> Returned from memory allocation >>> Copying log to new location >> >>> And then it hangs. I added a couple more print statements: >> >>> diff --git a/drivers/firmware/efi/libstub/tpm.c >> b/drivers/firmware/efi/libstub/tpm.c >>> index ee3fac109078..1ab5638bc50e 100644 >>> --- a/drivers/firmware/efi/libstub/tpm.c >>> +++ b/drivers/firmware/efi/libstub/tpm.c >>> @@ -148,8 +148,11 @@ void >> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) >>> efi_printk(sys_table_arg, "Copying log to new location\n"); >> >>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size); >>> + efi_printk(sys_table_arg, "Successfully memset log_tbl to 0\n"); >>> log_tbl->size = log_size; >>> + efi_printk(sys_table_arg, "Set log_tbl->size\n"); >>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2; >>> + efi_printk(sys_table_arg, "Set log_tbl-version\n"); >>> memcpy(log_tbl->log, (void *) first_entry_addr, log_size); >> >>> efi_printk(sys_table_arg, "Installing the log into the >> configuration table\n"); >> >>> and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) + log_size);" >> >> Thanks. Well, it looks like the memory that is supposedly allocated is not >> usable. I'm thinking this is a firmware bug. >> Ard, would you agree on this assumption? Thoughts on how to proceed? >> > > I am rather puzzled why the allocate_pool() should succeed and the > subsequent memset() should fail. This does not look like an issue that > is intimately related to TPM2 support, rather an issue in the firmware > that happens to get tickled after the change. > > Would you mind trying replacing EFI_LOADER_DATA with > EFI_BOOT_SERVICES_DATA in the allocate_pool() call? Replacing EFI_LOADER_DATA with EFI_BOOT_SERVICES_DATA still hangs at the memset() call. Regards, Jeremy
Re: Regression from efi: call get_event_log before ExitBootServices
On Fri, Mar 09, 2018 at 10:43:50AM +, Thiebaud Weksteen wrote: > Thanks a lot for trying out the patch! > > Please don't modify your install at this stage, I think we are hitting a > firmware bug and that would be awesome if we can fix how we are handling it. > So, if we reach that stage in the function it could either be that: > * The allocation did not succeed, somehow, but the firmware still returned > EFI_SUCCEED. > * The size requested is incorrect (I'm thinking something like a 1G of > log). This would be due to either a miscalculation of log_size (possible) > or; the returned values of GetEventLog are not correct. > I'm sending a patch to add checks for these. Could you please apply and > retest? > Again, thanks for helping debugging this. No problem, thanks for the help :) With the new patch: Locating the TCG2Protocol Calling GetEventLog on TCG2Protocol Log returned log_location is not empty log_size != 0 log_size < 1M Allocating memory for storing the logs Returned from memory allocation Copying log to new location And then it hangs. I added a couple more print statements: diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c index ee3fac109078..1ab5638bc50e 100644 --- a/drivers/firmware/efi/libstub/tpm.c +++ b/drivers/firmware/efi/libstub/tpm.c @@ -148,8 +148,11 @@ void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) efi_printk(sys_table_arg, "Copying log to new location\n"); memset(log_tbl, 0, sizeof(*log_tbl) + log_size); + efi_printk(sys_table_arg, "Successfully memset log_tbl to 0\n"); log_tbl->size = log_size; + efi_printk(sys_table_arg, "Set log_tbl->size\n"); log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2; + efi_printk(sys_table_arg, "Set log_tbl-version\n"); memcpy(log_tbl->log, (void *) first_entry_addr, log_size); efi_printk(sys_table_arg, "Installing the log into the configuration table\n"); and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) + log_size);" Regards, Jeremy
Re: Regression from efi: call get_event_log before ExitBootServices
On 03/08/2018 03:45 AM, Thiebaud Weksteen wrote: > Jeremy, Hans, could you both describe precisely how your boot is > configured? This feature is only triggered when booting the EFI stub of the > kernel so this may be not executed if you are using something else in > between. I put everything I know in the other sub-thread. > Jeremy, would you be able to modify the efi_retrieve_tpm2_eventlog_1_2 > function to add multiple efi_printk(sys_table_arg, "message\n"); to test: > if you get the output on your screen; and isolate which call might be the > cause of the hang? > I can forward a debug patch if that helps. Thanks for the patch, here's the output: Locating the TCG2Protocol Calling GetEventLog on TCG2Protocol Log returned log_location is not empty log_size != 0 Allocating memory for storing the logs Returned from memory allocation Copying log to new location At this point it hangs. Regards, Jeremy
Re: Regression from efi: call get_event_log before ExitBootServices
On 03/08/2018 11:50 AM, Hans de Goede wrote: > added these now> > > Hi, > > On 07-03-18 12:34, Javier Martinez Canillas wrote: >> Are you also able to read the TPM event logs? >> >> $ hexdump /sys/kernel/security/tpm0/binary_bios_measurements > > Yes for me that outputs a lot of hex :) For me, /sys/kernel/security/tmp0 doesn't exist on 4.15.6 or 4.16 with the patch reverted. >> The UEFI firmware does some measurements and so does shim. So you should >> have some event logs. What version of shim are you using? And also would >> be good to know if it's the same shim version that Jeremy is using. > > That is a very good question, I'm using: shim-ia32-13-0.7.x86_64, which is > the last version for F27 AFAICT. All my tablet has installed is shim-0.8-10.x86_64, no shim-ia32. > > But Jeremy's tablet might very well be not using the shim at all, as > I manually installed Fedora 25 on the tablet he now has, before Fedora > supported > machines with 32 bit EFI. I then later did a "dnf distro-sync" to > Fedora-27. > > Jeremy might also very well still be booting using a grub binary I build > manually back then, without any shim being involved. > > Jeremy what does efibootmgr -v output on your device ? # efibootmgr -v BootCurrent: 0003 Timeout: 4 seconds BootOrder: 0003,,0001,2001,2002,2003 Boot* Android X64 OS HD(1,GPT,215e6cf3-e97d-4735-9c4e-7338c8f5a645,0x800,0x32000)/File(\EFI\BOOT\bootx64.efi)RC Boot0001* Internal EFI Shell FvVol(a881d567-6cb0-4eee-8435-2e72d33e45b5)/FvFile(c57ad6b7-0515-40a8-9d21-551652854e37)RCM&". Boot0003* Fedora HD(1,GPT,215e6cf3-e97d-4735-9c4e-7338c8f5a645,0x800,0x32000)/File(\EFI\fedora\grubx64.efi) Boot2001* EFI USB DeviceRC Boot2002* EFI DVD/CDROM RC Boot2003* EFI Network RC Boot8087* Udm FvVol(a881d567-6cb0-4eee-8435-2e72d33e45b5)/FvFile(9a9ab4c1-ee1b-488b-b300-24544a7bd418) I think you're right about it using the old grub binary. I'm embarrassingly unfamiliar with both UEFI and grub, but I'm guessing you set the location of grub.cfg at compile time? When I boot \EFI\fedora\grubx64.efi, it's pulling the grub.cfg from \EFI\redhat\grub.cfg. Regards, Jeremy
[PATCH v2] scsi: sd: Keep disk read-only when re-reading partition
If the read-only flag is true on a SCSI disk, re-reading the partition table sets the flag back to false. To observe this bug, you can run: 1. blockdev --setro /dev/sda 2. blockdev --rereadpt /dev/sda 3. blockdev --getro /dev/sda This commit reads the disk's old state and combines it with the device disk-reported state rather than unconditionally marking it as RW. Reported-by: Li Ning Signed-off-by: Jeremy Cline --- Sorry about this, but there's a bug in the first version of this patch. I'm not sure what the protocol is for sending revised patches when the earlier version got accepted, but I don't see the first version in 4.16/scsi-fixes yet. If I should make a patch on top of the v1 patch, just let me know. Changes in v2: - Don't save the value of "get_disk_ro" to "sdkp->write_prot" since, as Li Ning noticed, that causes a second bug: when you make the disk writable again with "blockdev --setrw /dev/sda" it isn't possible to write to the disk until the partition is re-read. drivers/scsi/sd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index bff21e636ddd..3541caf3fceb 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -2595,6 +2595,7 @@ sd_read_write_protect_flag(struct scsi_disk *sdkp, unsigned char *buffer) int res; struct scsi_device *sdp = sdkp->device; struct scsi_mode_data data; + int disk_ro = get_disk_ro(sdkp->disk); int old_wp = sdkp->write_prot; set_disk_ro(sdkp->disk, 0); @@ -2635,7 +2636,7 @@ sd_read_write_protect_flag(struct scsi_disk *sdkp, unsigned char *buffer) "Test WP failed, assume Write Enabled\n"); } else { sdkp->write_prot = ((data.device_specific & 0x80) != 0); - set_disk_ro(sdkp->disk, sdkp->write_prot); + set_disk_ro(sdkp->disk, sdkp->write_prot || disk_ro); if (sdkp->first_scan || old_wp != sdkp->write_prot) { sd_printk(KERN_NOTICE, sdkp, "Write Protect is %s\n", sdkp->write_prot ? "on" : "off"); -- 2.16.2
Regression from efi: call get_event_log before ExitBootServices
Hi folks, Commit 33b6d03469b2 ("efi: call get_event_log before ExitBootServices") causes my GP-electronic T701 tablet to hang when booting. Reverting the patch series or hiding the TPM in the BIOS fixes the problem. I've never fiddled with TPMs before so I'm not sure what what debugging information to provide. It's got an Atom Z3735G and the UEFI firmware is InsydeH20 version BYT70A.YNCHENG.WIN.007. Regards, Jeremy
Re: [PATCH] scsi: sd: Keep disk read-only when re-reading partition
On 03/02/2018 07:00 AM, Johannes Thumshirn wrote: > On Thu, Mar 01, 2018 at 02:08:10PM +0000, Jeremy Cline wrote: >> If the read-only flag is true on a SCSI disk, re-reading the partition >> table sets the flag back to false. >> >> To observe this bug, you can run: >> >> 1. blockdev --setro /dev/sda >> 2. blockdev --rereadpt /dev/sda >> 3. blockdev --getro /dev/sda > > hi Jeremy, > > Do you mind wiring up a testcase in blktest [1] for this? Given that > you already have a rather trivial reproducer. > > [1] https://github.com/osandov/ Hi Johannes, Sure, I'll take care of it. Regards, Jeremy
[PATCH] scsi: sd: Keep disk read-only when re-reading partition
If the read-only flag is true on a SCSI disk, re-reading the partition table sets the flag back to false. To observe this bug, you can run: 1. blockdev --setro /dev/sda 2. blockdev --rereadpt /dev/sda 3. blockdev --getro /dev/sda This commit reads the disk's old state and combines it with the device disk-reported state rather than unconditionally marking it as RW. Reported-by: Li Ning Signed-off-by: Jeremy Cline --- drivers/scsi/sd.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index bff21e636ddd..7a3a66a7890f 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -2595,6 +2595,7 @@ sd_read_write_protect_flag(struct scsi_disk *sdkp, unsigned char *buffer) int res; struct scsi_device *sdp = sdkp->device; struct scsi_mode_data data; + int disk_ro = get_disk_ro(sdkp->disk); int old_wp = sdkp->write_prot; set_disk_ro(sdkp->disk, 0); @@ -2634,7 +2635,8 @@ sd_read_write_protect_flag(struct scsi_disk *sdkp, unsigned char *buffer) sd_first_printk(KERN_WARNING, sdkp, "Test WP failed, assume Write Enabled\n"); } else { - sdkp->write_prot = ((data.device_specific & 0x80) != 0); + sdkp->write_prot = ((data.device_specific & 0x80) != 0) || + disk_ro; set_disk_ro(sdkp->disk, sdkp->write_prot); if (sdkp->first_scan || old_wp != sdkp->write_prot) { sd_printk(KERN_NOTICE, sdkp, "Write Protect is %s\n", -- 2.16.2