| Message ID | 20240216163201.1901744-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: dvb-frontends: avoid stack overflow warnings with clang | expand |
Hi, On Fri, Feb 16, 2024 at 8:32 AM Arnd Bergmann <arnd@kernel.org> wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > A previous patch worked around a KASAN issue in stv0367, now a similar > problem showed up with clang: > > drivers/media/dvb-frontends/stv0367.c:1222:12: error: stack frame size (3624) exceeds limit (2048) in 'stv0367ter_set_frontend' [-Werror,-Wframe-larger-than] > 1214 | static int stv0367ter_set_frontend(struct dvb_frontend *fe) > > Rework the stv0367_writereg() function to be simpler and mark both > register access functions as noinline_for_stack so the temporary > i2c_msg structures do not get duplicated on the stack when KASAN_STACK > is enabled. > > Fixes: 3cd890dbe2a4 ("media: dvb-frontends: fix i2c access helpers for KASAN") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/media/dvb-frontends/stv0367.c | 34 +++++++-------------------- > 1 file changed, 8 insertions(+), 26 deletions(-) > > diff --git a/drivers/media/dvb-frontends/stv0367.c b/drivers/media/dvb-frontends/stv0367.c > index 48326434488c..72540ef4e5f8 100644 > --- a/drivers/media/dvb-frontends/stv0367.c > +++ b/drivers/media/dvb-frontends/stv0367.c > @@ -118,50 +118,32 @@ static const s32 stv0367cab_RF_LookUp2[RF_LOOKUP_TABLE2_SIZE][RF_LOOKUP_TABLE2_S > } > }; > > -static > -int stv0367_writeregs(struct stv0367_state *state, u16 reg, u8 *data, int len) > +static noinline_for_stack > +int stv0367_writereg(struct stv0367_state *state, u16 reg, u8 data) > { > - u8 buf[MAX_XFER_SIZE]; > + u8 buf[3] = { MSB(reg), LSB(reg), data }; > struct i2c_msg msg = { > .addr = state->config->demod_address, > .flags = 0, > .buf = buf, > - .len = len + 2 > + .len = 3, > }; > int ret; > > - if (2 + len > sizeof(buf)) { > - printk(KERN_WARNING > - "%s: i2c wr reg=%04x: len=%d is too big!\n", > - KBUILD_MODNAME, reg, len); > - return -EINVAL; > - } > - > - > - buf[0] = MSB(reg); > - buf[1] = LSB(reg); > - memcpy(buf + 2, data, len); I'm curious why a copy was made at all. Reviewed-by: Justin Stitt <justinstitt@google.com> > - > if (i2cdebug) > printk(KERN_DEBUG "%s: [%02x] %02x: %02x\n", __func__, > - state->config->demod_address, reg, buf[2]); > + state->config->demod_address, reg, data); > > ret = i2c_transfer(state->i2c, &msg, 1); > if (ret != 1) > printk(KERN_ERR "%s: i2c write error! ([%02x] %02x: %02x)\n", > - __func__, state->config->demod_address, reg, buf[2]); > + __func__, state->config->demod_address, reg, data); > > return (ret != 1) ? -EREMOTEIO : 0; > } > > -static int stv0367_writereg(struct stv0367_state *state, u16 reg, u8 data) > -{ > - u8 tmp = data; /* see gcc.gnu.org/bugzilla/show_bug.cgi?id=81715 */ > - > - return stv0367_writeregs(state, reg, &tmp, 1); > -} > - > -static u8 stv0367_readreg(struct stv0367_state *state, u16 reg) > +static noinline_for_stack > +u8 stv0367_readreg(struct stv0367_state *state, u16 reg) > { > u8 b0[] = { 0, 0 }; > u8 b1[] = { 0 }; > -- > 2.39.2 > Thanks Justin
On Fri, Feb 16, 2024, at 19:56, Justin Stitt wrote: > On Fri, Feb 16, 2024 at 8:32 AM Arnd Bergmann <arnd@kernel.org> wrote: >> - >> - buf[0] = MSB(reg); >> - buf[1] = LSB(reg); >> - memcpy(buf + 2, data, len); > > I'm curious why a copy was made at all. > > Reviewed-by: Justin Stitt <justinstitt@google.com> I guess the idea was to allow writing arbitrarily long contents to a single register with a single decriptor. The current version of the driver only ever writes a single byte value, but maybe it either used to have other writes, or it was meant to support them later but never did. Arnd
diff --git a/drivers/media/dvb-frontends/stv0367.c b/drivers/media/dvb-frontends/stv0367.c index 48326434488c..72540ef4e5f8 100644 --- a/drivers/media/dvb-frontends/stv0367.c +++ b/drivers/media/dvb-frontends/stv0367.c @@ -118,50 +118,32 @@ static const s32 stv0367cab_RF_LookUp2[RF_LOOKUP_TABLE2_SIZE][RF_LOOKUP_TABLE2_S } }; -static -int stv0367_writeregs(struct stv0367_state *state, u16 reg, u8 *data, int len) +static noinline_for_stack +int stv0367_writereg(struct stv0367_state *state, u16 reg, u8 data) { - u8 buf[MAX_XFER_SIZE]; + u8 buf[3] = { MSB(reg), LSB(reg), data }; struct i2c_msg msg = { .addr = state->config->demod_address, .flags = 0, .buf = buf, - .len = len + 2 + .len = 3, }; int ret; - if (2 + len > sizeof(buf)) { - printk(KERN_WARNING - "%s: i2c wr reg=%04x: len=%d is too big!\n", - KBUILD_MODNAME, reg, len); - return -EINVAL; - } - - - buf[0] = MSB(reg); - buf[1] = LSB(reg); - memcpy(buf + 2, data, len); - if (i2cdebug) printk(KERN_DEBUG "%s: [%02x] %02x: %02x\n", __func__, - state->config->demod_address, reg, buf[2]); + state->config->demod_address, reg, data); ret = i2c_transfer(state->i2c, &msg, 1); if (ret != 1) printk(KERN_ERR "%s: i2c write error! ([%02x] %02x: %02x)\n", - __func__, state->config->demod_address, reg, buf[2]); + __func__, state->config->demod_address, reg, data); return (ret != 1) ? -EREMOTEIO : 0; } -static int stv0367_writereg(struct stv0367_state *state, u16 reg, u8 data) -{ - u8 tmp = data; /* see gcc.gnu.org/bugzilla/show_bug.cgi?id=81715 */ - - return stv0367_writeregs(state, reg, &tmp, 1); -} - -static u8 stv0367_readreg(struct stv0367_state *state, u16 reg) +static noinline_for_stack +u8 stv0367_readreg(struct stv0367_state *state, u16 reg) { u8 b0[] = { 0, 0 }; u8 b1[] = { 0 };