| Message ID | 20210322160253.4032422-12-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | treewide: address gcc-11 -Wstringop-overread warnings | expand |
On Mon, 22 Mar 2021, Arnd Bergmann <arnd@kernel.org> wrote: > From: Arnd Bergmann <arnd@arndb.de> > > gcc-11 warns that intel_dp_check_mst_status() has a local array of > fourteen bytes and passes the last four bytes into a function that > expects a six-byte array: > > drivers/gpu/drm/i915/display/intel_dp.c: In function ‘intel_dp_check_mst_status’: > drivers/gpu/drm/i915/display/intel_dp.c:4556:22: error: ‘drm_dp_channel_eq_ok’ reading 6 bytes from a region of size 4 [-Werror=stringop-overread] > 4556 | !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) { > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/gpu/drm/i915/display/intel_dp.c:4556:22: note: referencing argument 1 of type ‘const u8 *’ {aka ‘const unsigned char *’} > In file included from drivers/gpu/drm/i915/display/intel_dp.c:38: > include/drm/drm_dp_helper.h:1459:6: note: in a call to function ‘drm_dp_channel_eq_ok’ > 1459 | bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE], > | ^~~~~~~~~~~~~~~~~~~~ > > Clearly something is wrong here, but I can't quite figure out what. > Changing the array size to 16 bytes avoids the warning, but is > probably the wrong solution here. Ugh. drm_dp_channel_eq_ok() does not actually require more than DP_LINK_STATUS_SIZE - 2 elements in the link_status. It's some other related functions that do, and in most cases it's convenient to read all those DP_LINK_STATUS_SIZE bytes. However, here the case is slightly different for DP MST, and the change causes reserved DPCD addresses to be read. Not sure it matters, but really I think the problem is what drm_dp_channel_eq_ok() advertizes. I also don't like the array notation with sizes in function parameters in general, because I think it's misleading. Would gcc-11 warn if a function actually accesses the memory out of bounds of the size? Anyway. I don't think we're going to get rid of the array notation anytime soon, if ever, no matter how much I dislike it, so I think the right fix would be to at least state the correct required size in drm_dp_channel_eq_ok(). BR, Jani. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 8c12d5375607..830e2515f119 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -65,7 +65,7 @@ > #include "intel_vdsc.h" > #include "intel_vrr.h" > > -#define DP_DPRX_ESI_LEN 14 > +#define DP_DPRX_ESI_LEN 16 > > /* DP DSC throughput values used for slice count calculations KPixels/s */ > #define DP_DSC_PEAK_PIXEL_RATE 2720000
On Thu, Mar 25, 2021 at 9:05 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > Clearly something is wrong here, but I can't quite figure out what. > > Changing the array size to 16 bytes avoids the warning, but is > > probably the wrong solution here. > > Ugh. drm_dp_channel_eq_ok() does not actually require more than > DP_LINK_STATUS_SIZE - 2 elements in the link_status. It's some other > related functions that do, and in most cases it's convenient to read all > those DP_LINK_STATUS_SIZE bytes. > > However, here the case is slightly different for DP MST, and the change > causes reserved DPCD addresses to be read. Not sure it matters, but > really I think the problem is what drm_dp_channel_eq_ok() advertizes. > > I also don't like the array notation with sizes in function parameters > in general, because I think it's misleading. Would gcc-11 warn if a > function actually accesses the memory out of bounds of the size? Yes, that is the point of the warning. Using an explicit length in an array argument type tells gcc that the function will never access beyond the end of that bound, and that passing a short array is a bug. I don't know if this /only/ means triggering a warning, or if gcc is also able to make optimizations after classifying this as undefined behavior that it would not make for an unspecified length. > Anyway. I don't think we're going to get rid of the array notation > anytime soon, if ever, no matter how much I dislike it, so I think the > right fix would be to at least state the correct required size in > drm_dp_channel_eq_ok(). Ok. Just to confirm: Changing the declaration to an unspecified length avoids the warnings, as does the patch below: diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index eedbb48815b7..6ebeec3d88a7 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -46,12 +46,12 @@ */ /* Helpers for DP link training */ -static u8 dp_link_status(const u8 link_status[DP_LINK_STATUS_SIZE], int r) +static u8 dp_link_status(const u8 link_status[DP_LINK_STATUS_SIZE - 2], int r) { return link_status[r - DP_LANE0_1_STATUS]; } -static u8 dp_get_lane_status(const u8 link_status[DP_LINK_STATUS_SIZE], +static u8 dp_get_lane_status(const u8 link_status[DP_LINK_STATUS_SIZE - 2], int lane) { int i = DP_LANE0_1_STATUS + (lane >> 1); @@ -61,7 +61,7 @@ static u8 dp_get_lane_status(const u8 link_status[DP_LINK_STATUS_SIZE], return (l >> s) & 0xf; } -bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE], +bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE - 2], int lane_count) { u8 lane_align; diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index edffd1dcca3e..160f7fd127b1 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -1456,7 +1456,7 @@ enum drm_dp_phy { #define DP_LINK_CONSTANT_N_VALUE 0x8000 #define DP_LINK_STATUS_SIZE 6 -bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE], +bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE - 2], int lane_count); bool drm_dp_clock_recovery_ok(const u8 link_status[DP_LINK_STATUS_SIZE], int lane_count); This obviously needs a good explanation in the code and the changelog text, which I don't have, but if the above is what you had in mind, please take that and add Reported-by/Tested-by: Arnd Bergmann <arnd@arndb.de>. Arnd
On 3/25/21 3:53 AM, Arnd Bergmann wrote: > On Thu, Mar 25, 2021 at 9:05 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: >>> Clearly something is wrong here, but I can't quite figure out what. >>> Changing the array size to 16 bytes avoids the warning, but is >>> probably the wrong solution here. >> >> Ugh. drm_dp_channel_eq_ok() does not actually require more than >> DP_LINK_STATUS_SIZE - 2 elements in the link_status. It's some other >> related functions that do, and in most cases it's convenient to read all >> those DP_LINK_STATUS_SIZE bytes. >> >> However, here the case is slightly different for DP MST, and the change >> causes reserved DPCD addresses to be read. Not sure it matters, but >> really I think the problem is what drm_dp_channel_eq_ok() advertizes. >> >> I also don't like the array notation with sizes in function parameters >> in general, because I think it's misleading. Would gcc-11 warn if a >> function actually accesses the memory out of bounds of the size? > > Yes, that is the point of the warning. Using an explicit length in an > array argument type tells gcc that the function will never access > beyond the end of that bound, and that passing a short array > is a bug. > > I don't know if this /only/ means triggering a warning, or if gcc > is also able to make optimizations after classifying this as undefined > behavior that it would not make for an unspecified length. GCC uses the array parameter notation as a hint for warnings but it doesn't optimize on this basis and never will be able to because code that accesses more elements from the array isn't invalid. Adding static to the bound, as in void f (int[static N]) does imply that the function won't access more than N elements and C intends for optimizers to rely on it, although GCC doesn't yet. The warning for the array notation is a more portable alternative to explicitly annotating functions with attribute access, and to -Wvla-parameter for VLA parameters. The latter seem to be used relatively rarely, sometimes deliberately because of the bad rap of VLA objects, even though VLA parameters don't suffer from the same problems. Martin > >> Anyway. I don't think we're going to get rid of the array notation >> anytime soon, if ever, no matter how much I dislike it, so I think the >> right fix would be to at least state the correct required size in >> drm_dp_channel_eq_ok(). > > Ok. Just to confirm: Changing the declaration to an unspecified length > avoids the warnings, as does the patch below: > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > index eedbb48815b7..6ebeec3d88a7 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -46,12 +46,12 @@ > */ > > /* Helpers for DP link training */ > -static u8 dp_link_status(const u8 link_status[DP_LINK_STATUS_SIZE], int r) > +static u8 dp_link_status(const u8 link_status[DP_LINK_STATUS_SIZE - 2], int r) > { > return link_status[r - DP_LANE0_1_STATUS]; > } > > -static u8 dp_get_lane_status(const u8 link_status[DP_LINK_STATUS_SIZE], > +static u8 dp_get_lane_status(const u8 link_status[DP_LINK_STATUS_SIZE - 2], > int lane) > { > int i = DP_LANE0_1_STATUS + (lane >> 1); > @@ -61,7 +61,7 @@ static u8 dp_get_lane_status(const u8 > link_status[DP_LINK_STATUS_SIZE], > return (l >> s) & 0xf; > } > > -bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE], > +bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE - 2], > int lane_count) > { > u8 lane_align; > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > index edffd1dcca3e..160f7fd127b1 100644 > --- a/include/drm/drm_dp_helper.h > +++ b/include/drm/drm_dp_helper.h > @@ -1456,7 +1456,7 @@ enum drm_dp_phy { > > #define DP_LINK_CONSTANT_N_VALUE 0x8000 > #define DP_LINK_STATUS_SIZE 6 > -bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE], > +bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE - 2], > int lane_count); > bool drm_dp_clock_recovery_ok(const u8 link_status[DP_LINK_STATUS_SIZE], > int lane_count); > > > This obviously needs a good explanation in the code and the changelog text, > which I don't have, but if the above is what you had in mind, please take that > and add Reported-by/Tested-by: Arnd Bergmann <arnd@arndb.de>. > > Arnd >
Hi, On 3/22/21 5:02 PM, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > gcc-11 warns that intel_dp_check_mst_status() has a local array of > fourteen bytes and passes the last four bytes into a function that > expects a six-byte array: > > drivers/gpu/drm/i915/display/intel_dp.c: In function ‘intel_dp_check_mst_status’: > drivers/gpu/drm/i915/display/intel_dp.c:4556:22: error: ‘drm_dp_channel_eq_ok’ reading 6 bytes from a region of size 4 [-Werror=stringop-overread] > 4556 | !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) { > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/gpu/drm/i915/display/intel_dp.c:4556:22: note: referencing argument 1 of type ‘const u8 *’ {aka ‘const unsigned char *’} > In file included from drivers/gpu/drm/i915/display/intel_dp.c:38: > include/drm/drm_dp_helper.h:1459:6: note: in a call to function ‘drm_dp_channel_eq_ok’ > 1459 | bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE], > | ^~~~~~~~~~~~~~~~~~~~ > > Clearly something is wrong here, but I can't quite figure out what. > Changing the array size to 16 bytes avoids the warning, but is > probably the wrong solution here. The drm displayport-helpers indeed expect a 6 bytes buffer, but they usually only consume 4 bytes. I don't think that changing the DP_DPRX_ESI_LEN is a good fix here, since it is used in multiple places, but the esi array already gets zero-ed out by its initializer, so we can just pass 2 extra 0 bytes to give drm_dp_channel_eq_ok() call the 6 byte buffer its prototype specifies by doing this: diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 897711d9d7d3..147962d4ad06 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -4538,7 +4538,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp) drm_WARN_ON_ONCE(&i915->drm, intel_dp->active_mst_links < 0); for (;;) { - u8 esi[DP_DPRX_ESI_LEN] = {}; + /* + * drm_dp_channel_eq_ok() expects a 6 byte large buffer, but + * the ESI info only contains 4 bytes, pass 2 extra 0 bytes. + */ + u8 esi[DP_DPRX_ESI_LEN + 2] = {}; bool handled; int retry; So i915 devs, would such a fix be acceptable ? Regards, Hans > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 8c12d5375607..830e2515f119 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -65,7 +65,7 @@ > #include "intel_vdsc.h" > #include "intel_vrr.h" > > -#define DP_DPRX_ESI_LEN 14 > +#define DP_DPRX_ESI_LEN 16 > > /* DP DSC throughput values used for slice count calculations KPixels/s */ > #define DP_DSC_PEAK_PIXEL_RATE 2720000 >
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 8c12d5375607..830e2515f119 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -65,7 +65,7 @@ #include "intel_vdsc.h" #include "intel_vrr.h" -#define DP_DPRX_ESI_LEN 14 +#define DP_DPRX_ESI_LEN 16 /* DP DSC throughput values used for slice count calculations KPixels/s */ #define DP_DSC_PEAK_PIXEL_RATE 2720000