[PATCH i-g-t v3 2/2] tests/intel/i915_pm_rpm: check drmModeGetConnector() before use

[Kamil Konieczny]

Hi Coelho,,
On 2024-06-12 at 14:10:56 +0000, Coelho, Luciano wrote:
> On Fri, 2024-06-07 at 17:11 +0200, Kamil Konieczny wrote:
> > Hi Luca,
> > On 2024-05-30 at 11:59:08 +0300, Luca Coelho wrote:
> > > The drmModeGetConnector() function can return NULL in some cases, so
> > > we need to check the return value before accessing it.  This is not
> > > being checked in init_mode_set_data(), so fix that.
> > > 
> > > From the bug report:
> > > 
> > > Starting subtest: module-reload
> > > Received signal SIGSEGV.
> > > Stack trace:
> > 
> > Now I think that line 'Stack trace' is not needed, only those
> > two with subtest name and signal are enough.
> 
> Okay... it's mostly noise IMHO, anyway.
> 
> 
> > > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10911
> > > Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > > Cc: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
> > > Cc: Jani Saarinen <jani.saarinen at intel.com>
> > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
> > > Signed-off-by: Luca Coelho <luciano.coelho at intel.com>
> > > ---
> > >  tests/intel/i915_pm_rpm.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/tests/intel/i915_pm_rpm.c b/tests/intel/i915_pm_rpm.c
> > > index 0ea5fbd8afd9..e0f395fa8a84 100644
> > > --- a/tests/intel/i915_pm_rpm.c
> > > +++ b/tests/intel/i915_pm_rpm.c
> > > @@ -515,6 +515,12 @@ static void init_mode_set_data(struct mode_set_data *data)
> > >  			data->connectors[i] =
> > >  				drmModeGetConnector(drm_fd,
> > >  						    data->res->connectors[i]);
> > > +			if (!data->connectors[i]) {
> > > +				igt_warn("Could not read connector %u\n",
> > ----------------^^^^^^^^
> > igt_debug
> > 
> > > +					 data->res->connectors[i]);
> > > +				continue;
> > > +			}
> > > +
> > 
> > We generally do not warn and fail a test in case some prerequistics
> > are not present, that is why it is better to igt_debug here
> 
> This is not about pre-requisites.  This is about a race condition where
> we get a larger number of connectors than what is actually available
> later.

You didn't write that in a comment in code nor in commit message,
looks like it could help later on when this happen.

> 
> I think the test should fail in this case, because then we will look at
> what is going on and try to figure out what's causing it.
> 
> I had igt_warn(), then you told me to change to igt_debug(), then you
> told me to change it back to igt_warn() and now you're asking me to
> change it to igt_debug, *again*?!
> 

Nevermind, let it be warn.

> 
> > and make checks in few other places, like:
> > 			if (data->connectors[i]) {
> >                     // here we can safely use/free connector or edid
> >             }
> > or
> > 			if (!data->connectors[i])
> >                 continue;
> 
> I don't get it.  Can you explain in more details?
>

I did a check locally and it seems NULLs there are not a problem,
so your change seems ok,

Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>

Regards,
Kamil

> 
> > grep -n connectors tests/intel/i915_pm_rpm.c
> > 139:    drmModeConnectorPtr connectors[MAX_CONNECTORS];
> > 151:    drmModeConnectorPtr connectors[MAX_CONNECTORS];
> > 319:    for (int i = 0; i < data->res->count_connectors; i++) {
> > 320:            drmModeConnectorPtr c = data->connectors[i];
> > 513:            igt_assert(data->res->count_connectors <= MAX_CONNECTORS);
> > 514:            for (int i = 0; i < data->res->count_connectors; i++) {
> > 515:                    data->connectors[i] =
> > 517:                                                data->res->connectors[i]);
> > 518:                    data->edids[i] = get_connector_edid(data->connectors[i], i);
> > 531:            for (int i = 0; i < data->res->count_connectors; i++) {
> > 532:                    drmModeFreeConnector(data->connectors[i]);
> > 
> > so all you need is add checks in lines after 320 and 531.
> 
> I don't think it's needed in 320, because with my change we're already
> checking if the pointer was NULL when we _assigned_
> data->connectors[i].  We should never get here with a NULL in the
> table.
> 
> My change is about checking the return value of drmModeGetConnector(),
> which can return NULL, so I checked all the callers and made sure they
> check the return value.
> 
> 
> > Btw test is missing final fixture with i915 module load but that
> > should be fixed in separate patch.
> 
> Okay.
> 
> --
> Cheers,
> Luca.