| Message ID | 20190309105441.18472-1-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | i2c: add extra check to safe DMA buffer helper | expand |
On Sat, Mar 9, 2019 at 6:54 PM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > Make sure we report no buffer for 0-length messages. This can only > happen if threshold is set to 0 which is kind of bogus but we should > handle this situation nonetheless. > > Reported-by: Hsin-Yi Wang <hsinyi@chromium.org> > Fixes: e94bc5d18be0 ("i2c: add helpers to ease DMA handling") > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/i2c/i2c-core-base.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index cb6c5cb0df0b..22c8f38bf68c 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -2268,7 +2268,8 @@ EXPORT_SYMBOL(i2c_put_adapter); > */ > u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold) > { How about also printing some warning if threshold is zero? Since caller might expect that if msg->len >= threshold, they will get valid pointer. But they will get valid pointer if (msg->len, threshold) is (>0, 0) and NULL if (=0, 0). So warn them that the threshold is bogus. > - if (msg->len < threshold) > + /* also skip 0-length msgs for bogus thresholds of 0 */ > + if (msg->len < threshold || msg->len == 0) > return NULL; > > if (msg->flags & I2C_M_DMA_SAFE) > -- > 2.19.1 >
> How about also printing some warning if threshold is zero? Since Hmm, not a warning because a user of a kernel can't do much about it. A debug might make sense; it is for developers and will be compiled away for production use. > caller might expect that if msg->len >= threshold, they will get valid > pointer. But they will get valid pointer if (msg->len, threshold) is > (>0, 0) and NULL if (=0, 0). So warn them that the threshold is bogus. Well, callers should always check for NULL because a valid pointer can not be guaranteed (ENOMEM case). And I still don't get why would you want a valid DMA-able pointer if the msg length is 0? Thanks!
On Tue, Mar 12, 2019 at 2:46 AM Wolfram Sang <wsa@the-dreams.de> wrote: > > > > How about also printing some warning if threshold is zero? Since > > Hmm, not a warning because a user of a kernel can't do much about it. A > debug might make sense; it is for developers and will be compiled away > for production use. > > > caller might expect that if msg->len >= threshold, they will get valid > > pointer. But they will get valid pointer if (msg->len, threshold) is > > (>0, 0) and NULL if (=0, 0). So warn them that the threshold is bogus. > > Well, callers should always check for NULL because a valid pointer can > not be guaranteed (ENOMEM case). And I still don't get why would you > want a valid DMA-able pointer if the msg length is 0? Yes. But I think the debug message here is just to tell caller that threshold == 0 might not make sense. For threshold > 0, msg->len >= threshold returns valid. But it's not the case when threshold is 0. Also, threshold is 0 and 1 behaves the same: msg->len == 0 returns NULL, msg->len > 0 returns valid. > > Thanks! > > -----BEGIN PGP SIGNATURE----- > > iQIzBAABCAAdFiEEOZGx6rniZ1Gk92RdFA3kzBSgKbYFAlyGrQIACgkQFA3kzBSg > Kbb61Q//XZoT6dJYvO2WdyhYQf2Zpb4S1JEwSkMxa1XRQmw4caIdOLyJVt+OPzFo > 6Swbg0GQbFPV0tagj9VRTTWWTA6biRf69t+d5fNCMFWJ4Rx89CuJvy1EOA6A8LHN > Ckce/8HUXJAFdIaMwlkCagwepfO3WJ7GYDrGxiOM9e8cCOGkNgw/bXe3Xks2rcgC > 8eFiUGqDrrG3LwsTlqBfjBC1aErwYgC/QBO0uhWiodkszggEV8T1D9ykbfTSOvCr > LHZro3jzmR8E+K1zsujXuP2eDAhaN5V1s7VNI2mrzsQcxl/v7YKd17Us5Iqzq/1d > 4Ynv0gr7kTLoq5wOv5gamESXQ+m8PBmU7gikrIBh7ie0HXXyQY1U7sO5iSwBoMwC > eMegBAK8IDr5qM2YAb+urQQ/w7NdA731cYpm0+srfVSsxdpCvWJSROXHjRcrBzkm > xkw9fY0s5l5hWzWtjrOocYVd68MOmj6hnVFW6n9itAuRsi7CZJ4UnxxpcCbBgfEQ > +fhru8D6Ie1VkUtLtQlNFh61KJDTZ1Sg2YisL1M1N/nRAH22pSaMxyX2kpXsZI7f > WsCVIj3Rgow6vOqqUa85aYOqcyWllO29AMTMu+srAbfUWTIZ6iTtB5Dz5bC9oUPN > YyfviJYySO6UocuuZH1AAmvPdoqlly11ouTiA69vlPIx0hW8o9U= > =ZJU8 > -----END PGP SIGNATURE-----
> Yes. But I think the debug message here is just to tell caller that > threshold == 0 might not make sense. I can agree to the debug message. I'll send V2 soon.
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index cb6c5cb0df0b..22c8f38bf68c 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -2268,7 +2268,8 @@ EXPORT_SYMBOL(i2c_put_adapter); */ u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold) { - if (msg->len < threshold) + /* also skip 0-length msgs for bogus thresholds of 0 */ + if (msg->len < threshold || msg->len == 0) return NULL; if (msg->flags & I2C_M_DMA_SAFE)
Make sure we report no buffer for 0-length messages. This can only happen if threshold is set to 0 which is kind of bogus but we should handle this situation nonetheless. Reported-by: Hsin-Yi Wang <hsinyi@chromium.org> Fixes: e94bc5d18be0 ("i2c: add helpers to ease DMA handling") Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/i2c/i2c-core-base.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)