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

["Coelho, Luciano" <luciano.coelho@intel.com>]

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@linux.intel.com>
> > Cc: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> > Cc: Jani Saarinen <jani.saarinen@intel.com>
> > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > Signed-off-by: Luca Coelho <luciano.coelho@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.

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*?!


> 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?


> 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.