Hi,
On 28/08/2024 10:41, Philipp Stanner wrote:
drm_sched_job_init() has no control over how users allocate struct
drm_sched_job. Unfortunately, the function can also not set some struct
members such as job->sched.
job->sched usage from within looks like a bug. But not related to the
memset you add.
For this one something like this looks easiest for a start:
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index ab53ab486fe6..877113b01af2 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -788,7 +788,7 @@ int drm_sched_job_init(struct drm_sched_job *job,
* or worse--a blank screen--leave a trail in the
* logs, so this can be debugged easier.
*/
- drm_err(job->sched, "%s: entity has no rq!\n", __func__);
+ pr_err("%s: entity has no rq!\n", __func__);
return -ENOENT;
}
Fixes: 56e449603f0a ("drm/sched: Convert the GPU scheduler to variable
number of run-queues")
Cc: <stable@xxxxxxxxxxxxxxx> # v6.7+
This could theoretically lead to UB by users dereferencing the struct's
pointer members too early.
Hmm if drm_sched_job_init returned an error callers should not
dereference anything. What was actually the issue you were debugging?
Adding a memset is I think not the best solution since it is very likely
redundant to someone doing a kzalloc in the first place.
Regards,
Tvrtko
It is easier to debug such issues if these pointers are initialized to
NULL, so dereferencing them causes a NULL pointer exception.
Accordingly, drm_sched_entity_init() does precisely that and initializes
its struct with memset().
Initialize parameter "job" to 0 in drm_sched_job_init().
Signed-off-by: Philipp Stanner <pstanner@xxxxxxxxxx>
---
No changes in v2.
---
drivers/gpu/drm/scheduler/sched_main.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 356c30fa24a8..b0c8ad10b419 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -806,6 +806,14 @@ int drm_sched_job_init(struct drm_sched_job *job,
return -EINVAL;
}
+ /*
+ * We don't know for sure how the user has allocated. Thus, zero the
+ * struct so that unallowed (i.e., too early) usage of pointers that
+ * this function does not set is guaranteed to lead to a NULL pointer
+ * exception instead of UB.
+ */
+ memset(job, 0, sizeof(*job));
+
job->entity = entity;
job->credits = credits;
job->s_fence = drm_sched_fence_alloc(entity, owner);
