All of lore.kernel.org
 help / color / mirror / Atom feed
From: Douglas Anderson <dianders@chromium.org>
To: amasule@codeaurora.org, stanimir.varbanov@linaro.org
Cc: swboyd@chromium.org, jkardatzke@google.com, mka@chromium.org,
	Douglas Anderson <dianders@chromium.org>,
	Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-media@vger.kernel.org
Subject: [RFC PATCH] media: venus: Fix NULL pointer dereference in core selection
Date: Mon,  1 Jun 2020 15:03:22 -0700	[thread overview]
Message-ID: <20200601150314.RFC.1.I1e40623bbe8fa43ff1415fc273cba66503b9b048@changeid> (raw)

The newly-introduced function min_loaded_core() iterates over all of
the venus instances an tries to figure out how much load each instance
is putting on each core.  Not all instances, however, might be fully
initialized.  Specifically the "codec_freq_data" is initialized as
part of vdec_queue_setup(), but an instance may already be in the list
of all instances before that time.

Let's band-aid this by checking to see if codec_freq_data is NULL
before dereferencing.

NOTE: without this fix I was running into a crash.  Specifically there
were two venus instances.  One was doing start_streaming.  The other
was midway through queue_setup but hadn't yet gotten to initting
"codec_freq_data".

Fixes: eff82f79c562 ("media: venus: introduce core selection")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
I'm not massively happy about this commit but it's the best I could
come up with without being much more of an expert in the venus codec.
If someone has a better patch then please just consider this one to be
a bug report and feel free to submit a better fix!  :-)

In general I wonder a little bit about whether it's safe to be peeking
at all the instances without grabbing the "inst->lock" on each one.  I
guess it is since we do it both here and in load_scale_v4() but I
don't know why.

One thought I had was that we could fully avoid accessing the other
instances, at least in min_loaded_core(), by just keeping track of
"core1_load" and "core2_load" in "struct venus_core".  Whenever we add
a new instance we could add to the relevant variables and whenever we
release an instance we could remove.  Such a change seems cleaner but
would require someone to test to make sure we didn't miss any case
(AKA we always properly added/removed our load from the globals).

 drivers/media/platform/qcom/venus/pm_helpers.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index abf93158857b..a1d998f62cf2 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -496,6 +496,8 @@ min_loaded_core(struct venus_inst *inst, u32 *min_coreid, u32 *min_load)
 	list_for_each_entry(inst_pos, &core->instances, list) {
 		if (inst_pos == inst)
 			continue;
+		if (!inst_pos->clk_data.codec_freq_data)
+			continue;
 		vpp_freq = inst_pos->clk_data.codec_freq_data->vpp_freq;
 		coreid = inst_pos->clk_data.core_id;
 
-- 
2.27.0.rc2.251.g90737beb825-goog


             reply	other threads:[~2020-06-01 22:03 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-01 22:03 Douglas Anderson [this message]
2020-06-02  3:39 ` [RFC PATCH] media: venus: Fix NULL pointer dereference in core selection Doug Anderson
2020-06-22 11:51   ` Stanimir Varbanov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200601150314.RFC.1.I1e40623bbe8fa43ff1415fc273cba66503b9b048@changeid \
    --to=dianders@chromium.org \
    --cc=agross@kernel.org \
    --cc=amasule@codeaurora.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=jkardatzke@google.com \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=mka@chromium.org \
    --cc=stanimir.varbanov@linaro.org \
    --cc=swboyd@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.