tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head: 575115360652e9920cc56a028a286ebe9bf82694
commit: c664e2137a27680922d8aeb64fb10313416b254f can: etas_es58x: add support for the ETAS ES58X_FD CAN USB interfaces
date: 11 months ago
compiler: powerpc64-linux-gcc (GCC) 11.2.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
cppcheck possible warnings: (new ones prefixed by >>, may not be real problems)
In file included from drivers/net/can/usb/etas_es58x/es58x_fd.c:
>> drivers/net/can/usb/etas_es58x/es58x_fd.c:174:8: warning: Uninitialized variable: rx_event_msg [uninitvar]
ret = es58x_check_msg_len(es58x_dev->dev, *rx_event_msg, msg_len);
^
vim +174 drivers/net/can/usb/etas_es58x/es58x_fd.c
c664e2137a2768 Vincent Mailhol 2021-04-10 165
c664e2137a2768 Vincent Mailhol 2021-04-10 166 static int es58x_fd_rx_event_msg(struct net_device *netdev,
c664e2137a2768 Vincent Mailhol 2021-04-10 167 const struct es58x_fd_urb_cmd *es58x_fd_urb_cmd)
c664e2137a2768 Vincent Mailhol 2021-04-10 168 {
c664e2137a2768 Vincent Mailhol 2021-04-10 169 struct es58x_device *es58x_dev = es58x_priv(netdev)->es58x_dev;
c664e2137a2768 Vincent Mailhol 2021-04-10 170 u16 msg_len = get_unaligned_le16(&es58x_fd_urb_cmd->msg_len);
c664e2137a2768 Vincent Mailhol 2021-04-10 @171 const struct es58x_fd_rx_event_msg *rx_event_msg;
c664e2137a2768 Vincent Mailhol 2021-04-10 172 int ret;
c664e2137a2768 Vincent Mailhol 2021-04-10 173
c664e2137a2768 Vincent Mailhol 2021-04-10 @174 ret = es58x_check_msg_len(es58x_dev->dev, *rx_event_msg, msg_len);
c664e2137a2768 Vincent Mailhol 2021-04-10 175 if (ret)
c664e2137a2768 Vincent Mailhol 2021-04-10 176 return ret;
c664e2137a2768 Vincent Mailhol 2021-04-10 177
c664e2137a2768 Vincent Mailhol 2021-04-10 178 rx_event_msg = &es58x_fd_urb_cmd->rx_event_msg;
c664e2137a2768 Vincent Mailhol 2021-04-10 179
c664e2137a2768 Vincent Mailhol 2021-04-10 180 return es58x_rx_err_msg(netdev, rx_event_msg->error_code,
c664e2137a2768 Vincent Mailhol 2021-04-10 181 rx_event_msg->event_code,
c664e2137a2768 Vincent Mailhol 2021-04-10 182 get_unaligned_le64(&rx_event_msg->timestamp));
c664e2137a2768 Vincent Mailhol 2021-04-10 183 }
c664e2137a2768 Vincent Mailhol 2021-04-10 184
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Wed. 2 Mar 2022 at 19:32, Marc Kleine-Budde <[email protected]> wrote:
> On 02.03.2022 17:47:08, kernel test robot wrote:
> > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > head: 575115360652e9920cc56a028a286ebe9bf82694
> > commit: c664e2137a27680922d8aeb64fb10313416b254f can: etas_es58x: add support for the ETAS ES58X_FD CAN USB interfaces
> > date: 11 months ago
> > compiler: powerpc64-linux-gcc (GCC) 11.2.0
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <[email protected]>
Do we still need the tag for false positives (c.f. below)?
I am fine to add it, but maybe this will mess with your statistics?
> >
> > cppcheck possible warnings: (new ones prefixed by >>, may not be real problems)
Indeed, not a real problem. My wild guess is that cppcheck does
not recognize __stringify() as a pre-procesor macro. That would
not be the first time a static analyzer would fail on
it. checkpatch had the same issue which I fixed in:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7b844345fc2a9c46f8bb8cdb7408c766dfcdd83d
Or maybe cppcheck does not try to expand the macro and directly
yield a warning?
Overall, I think that GCC/Clang already does a good job at
finding usage of uninitialized variable. If cppcheck is less
mature in this aspect, I suggest to deactivate this particular
cppcheck rule and leave this job to GCC/Clang's -Wuninitialized.
> > In file included from drivers/net/can/usb/etas_es58x/es58x_fd.c:
> > >> drivers/net/can/usb/etas_es58x/es58x_fd.c:174:8: warning: Uninitialized variable: rx_event_msg [uninitvar]
> > ret = es58x_check_msg_len(es58x_dev->dev, *rx_event_msg, msg_len);
> > ^
> >
> > vim +174 drivers/net/can/usb/etas_es58x/es58x_fd.c
> >
> > c664e2137a2768 Vincent Mailhol 2021-04-10 165
> > c664e2137a2768 Vincent Mailhol 2021-04-10 166 static int es58x_fd_rx_event_msg(struct net_device *netdev,
> > c664e2137a2768 Vincent Mailhol 2021-04-10 167 const struct es58x_fd_urb_cmd *es58x_fd_urb_cmd)
> > c664e2137a2768 Vincent Mailhol 2021-04-10 168 {
> > c664e2137a2768 Vincent Mailhol 2021-04-10 169 struct es58x_device *es58x_dev = es58x_priv(netdev)->es58x_dev;
> > c664e2137a2768 Vincent Mailhol 2021-04-10 170 u16 msg_len = get_unaligned_le16(&es58x_fd_urb_cmd->msg_len);
> > c664e2137a2768 Vincent Mailhol 2021-04-10 @171 const struct es58x_fd_rx_event_msg *rx_event_msg;
> > c664e2137a2768 Vincent Mailhol 2021-04-10 172 int ret;
> > c664e2137a2768 Vincent Mailhol 2021-04-10 173
> > c664e2137a2768 Vincent Mailhol 2021-04-10 @174 ret = es58x_check_msg_len(es58x_dev->dev, *rx_event_msg, msg_len);
> > c664e2137a2768 Vincent Mailhol 2021-04-10 175 if (ret)
> > c664e2137a2768 Vincent Mailhol 2021-04-10 176 return ret;
> > c664e2137a2768 Vincent Mailhol 2021-04-10 177
> > c664e2137a2768 Vincent Mailhol 2021-04-10 178 rx_event_msg = &es58x_fd_urb_cmd->rx_event_msg;
> > c664e2137a2768 Vincent Mailhol 2021-04-10 179
> > c664e2137a2768 Vincent Mailhol 2021-04-10 180 return es58x_rx_err_msg(netdev, rx_event_msg->error_code,
> > c664e2137a2768 Vincent Mailhol 2021-04-10 181 rx_event_msg->event_code,
> > c664e2137a2768 Vincent Mailhol 2021-04-10 182 get_unaligned_le64(&rx_event_msg->timestamp));
> > c664e2137a2768 Vincent Mailhol 2021-04-10 183 }
> > c664e2137a2768 Vincent Mailhol 2021-04-10 184
>
> Thanks for the report.
>
> This looks like a false positive to me, as es58x_check_msg_len() is not
> a function, but a macro:
>
> | #define es58x_check_msg_len(dev, msg, actual_len) \
> | __es58x_check_msg_len(dev, __stringify(msg), \
> | actual_len, sizeof(msg))
>
> __es58x_check_msg_len() don't use "rx_event_msg" directly, but only a
> string representation of it and a "sizeof()".
Ack.
> I think it's possible to assign rx_event_msg before the
> es58x_check_msg_len().
Yes, I will do so. Even if this is a false positive, this pattern
can be misleading. e.g. during a code review, this does indeed
look incorrect at first glance.
Also, doing such change would be consistent with was is done in
other functions:
https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/etas_es58x/es58x_fd.c#L210
This not being a bug fix, is it fine to send it to net-next?
Or do you see a need to backport this?
> I think (hope?) the compiler will not optimize
> anything away. :)
With a function call and a return statement, the compiler would
need to be severely defective to try to optimize this away :)
> regards,
> Marc
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Embedded Linux | https://www.pengutronix.de |
> Vertretung West/Dortmund | Phone: +49-231-2826-924 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 02.03.2022 17:47:08, kernel test robot wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head: 575115360652e9920cc56a028a286ebe9bf82694
> commit: c664e2137a27680922d8aeb64fb10313416b254f can: etas_es58x: add support for the ETAS ES58X_FD CAN USB interfaces
> date: 11 months ago
> compiler: powerpc64-linux-gcc (GCC) 11.2.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[email protected]>
>
>
> cppcheck possible warnings: (new ones prefixed by >>, may not be real problems)
>
> In file included from drivers/net/can/usb/etas_es58x/es58x_fd.c:
> >> drivers/net/can/usb/etas_es58x/es58x_fd.c:174:8: warning: Uninitialized variable: rx_event_msg [uninitvar]
> ret = es58x_check_msg_len(es58x_dev->dev, *rx_event_msg, msg_len);
> ^
>
> vim +174 drivers/net/can/usb/etas_es58x/es58x_fd.c
>
> c664e2137a2768 Vincent Mailhol 2021-04-10 165
> c664e2137a2768 Vincent Mailhol 2021-04-10 166 static int es58x_fd_rx_event_msg(struct net_device *netdev,
> c664e2137a2768 Vincent Mailhol 2021-04-10 167 const struct es58x_fd_urb_cmd *es58x_fd_urb_cmd)
> c664e2137a2768 Vincent Mailhol 2021-04-10 168 {
> c664e2137a2768 Vincent Mailhol 2021-04-10 169 struct es58x_device *es58x_dev = es58x_priv(netdev)->es58x_dev;
> c664e2137a2768 Vincent Mailhol 2021-04-10 170 u16 msg_len = get_unaligned_le16(&es58x_fd_urb_cmd->msg_len);
> c664e2137a2768 Vincent Mailhol 2021-04-10 @171 const struct es58x_fd_rx_event_msg *rx_event_msg;
> c664e2137a2768 Vincent Mailhol 2021-04-10 172 int ret;
> c664e2137a2768 Vincent Mailhol 2021-04-10 173
> c664e2137a2768 Vincent Mailhol 2021-04-10 @174 ret = es58x_check_msg_len(es58x_dev->dev, *rx_event_msg, msg_len);
> c664e2137a2768 Vincent Mailhol 2021-04-10 175 if (ret)
> c664e2137a2768 Vincent Mailhol 2021-04-10 176 return ret;
> c664e2137a2768 Vincent Mailhol 2021-04-10 177
> c664e2137a2768 Vincent Mailhol 2021-04-10 178 rx_event_msg = &es58x_fd_urb_cmd->rx_event_msg;
> c664e2137a2768 Vincent Mailhol 2021-04-10 179
> c664e2137a2768 Vincent Mailhol 2021-04-10 180 return es58x_rx_err_msg(netdev, rx_event_msg->error_code,
> c664e2137a2768 Vincent Mailhol 2021-04-10 181 rx_event_msg->event_code,
> c664e2137a2768 Vincent Mailhol 2021-04-10 182 get_unaligned_le64(&rx_event_msg->timestamp));
> c664e2137a2768 Vincent Mailhol 2021-04-10 183 }
> c664e2137a2768 Vincent Mailhol 2021-04-10 184
Thanks for the report.
This looks like a false positive to me, as es58x_check_msg_len() is not
a function, but a macro:
| #define es58x_check_msg_len(dev, msg, actual_len) \
| __es58x_check_msg_len(dev, __stringify(msg), \
| actual_len, sizeof(msg))
__es58x_check_msg_len() don't use "rx_event_msg" directly, but only a
string representation of it and a "sizeof()".
I think it's possible to assign rx_event_msg before the
es58x_check_msg_len(). I think (hope?) the compiler will not optimize
anything away. :)
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Wed. 2 Mar 2022 at 22:04, Marc Kleine-Budde <[email protected]> wrote:
> I was thinking of this:
>
> | void *foo = bar->baz;
> |
> | if (!bar)
> | return;
> |
> | printf("%p", foo);
>
> There were/are compilers that optimize the bar NULL pointer check away,
> because bar has already been de-referenced.
Sorry, I do not get your example. If bar is NULL,
| void *foo = bar->baz;
would segfault and thus the check is not reached.
If bar is not NULL, the check succeeds.
In both cases, the return statement of the if branch is never
executed making this some dead code. So I do not see why this is
an issue if the compiler removes it.
Yours sincerely,
Vincent Mailhol
On 02.03.2022 21:49:27, Vincent MAILHOL wrote:
> > I think it's possible to assign rx_event_msg before the
> > es58x_check_msg_len().
>
> Yes, I will do so. Even if this is a false positive, this pattern
> can be misleading. e.g. during a code review, this does indeed
> look incorrect at first glance.
>
> Also, doing such change would be consistent with was is done in
> other functions:
> https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/etas_es58x/es58x_fd.c#L210
>
> This not being a bug fix, is it fine to send it to net-next?
ACK
> Or do you see a need to backport this?
Don't think so.
> > I think (hope?) the compiler will not optimize
> > anything away. :)
>
> With a function call and a return statement, the compiler would
> need to be severely defective to try to optimize this away :)
I was thinking of this:
| void *foo = bar->baz;
|
| if (!bar)
| return;
|
| printf("%p", foo);
There were/are compilers that optimize the bar NULL pointer check away,
because bar has already been de-referenced.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 02.03.2022 23:42:53, Vincent MAILHOL wrote:
> On Wed. 2 Mar 2022 at 22:04, Marc Kleine-Budde <[email protected]> wrote:
> > I was thinking of this:
> >
> > | void *foo = bar->baz;
> > |
> > | if (!bar)
> > | return;
> > |
> > | printf("%p", foo);
> >
> > There were/are compilers that optimize the bar NULL pointer check away,
> > because bar has already been de-referenced.
>
> Sorry, I do not get your example. If bar is NULL,
> | void *foo = bar->baz;
> would segfault and thus the check is not reached.
ACK
> If bar is not NULL, the check succeeds.
>
> In both cases, the return statement of the if branch is never
> executed making this some dead code. So I do not see why this is
> an issue if the compiler removes it.
IIRC in some cases the code was shuffled around by the compiler and the
NULL pointer check was done....and with a new compiler version it
stopped working :)
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |