From: Marc Kleine-Budde <mkl@pengutronix.de>
To: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Cc: linux-can <linux-can@vger.kernel.org>,
"Stefan Mätje" <Stefan.Maetje@esd.eu>,
netdev <netdev@vger.kernel.org>,
"open list" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 5/7] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC)
Date: Wed, 18 Aug 2021 10:19:34 +0200 [thread overview]
Message-ID: <20210818081934.6f23ghoom2dkv53m@pengutronix.de> (raw)
In-Reply-To: <CAMZ6RqLj94UU_b8dDAzinVsLaV6pBR-cWbHmjwGhx3vfWiKt_g@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4180 bytes --]
On 18.08.2021 17:08:51, Vincent MAILHOL wrote:
> On Wed 18 Aug 2021 at 04:55, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 15.08.2021 12:32:46, Vincent Mailhol wrote:
> > > +static int can_tdc_changelink(struct net_device *dev, const struct nlattr *nla,
> > > + struct netlink_ext_ack *extack)
> > > +{
> > > + struct nlattr *tb_tdc[IFLA_CAN_TDC_MAX + 1];
> > > + struct can_priv *priv = netdev_priv(dev);
> > > + struct can_tdc *tdc = &priv->tdc;
> > > + const struct can_tdc_const *tdc_const = priv->tdc_const;
> > > + int err;
> > > +
> > > + if (!tdc_const || !can_tdc_is_enabled(priv))
> > > + return -EOPNOTSUPP;
> > > +
> > > + if (dev->flags & IFF_UP)
> > > + return -EBUSY;
> > > +
> > > + err = nla_parse_nested(tb_tdc, IFLA_CAN_TDC_MAX, nla,
> > > + can_tdc_policy, extack);
> > > + if (err)
> > > + return err;
> > > +
> > > + if (tb_tdc[IFLA_CAN_TDC_TDCV]) {
> > > + u32 tdcv = nla_get_u32(tb_tdc[IFLA_CAN_TDC_TDCV]);
> > > +
> > > + if (tdcv < tdc_const->tdcv_min || tdcv > tdc_const->tdcv_max)
> > > + return -EINVAL;
> > > +
> > > + tdc->tdcv = tdcv;
> >
> > You have to assign to a temporary struct first, and set the priv->tdc
> > after complete validation, otherwise you end up with inconsistent
> > values.
>
> Actually, copying the temporary structure to priv->tdc is not an
> atomic operation. Here, you are only reducing the window, not
> closing it.
It's not a race I'm fixing.
>
> > > + }
> > > +
> > > + if (tb_tdc[IFLA_CAN_TDC_TDCO]) {
> > > + u32 tdco = nla_get_u32(tb_tdc[IFLA_CAN_TDC_TDCO]);
> > > +
> > > + if (tdco < tdc_const->tdco_min || tdco > tdc_const->tdco_max)
> > > + return -EINVAL;
> > > +
> > > + tdc->tdco = tdco;
> > > + }
> > > +
> > > + if (tb_tdc[IFLA_CAN_TDC_TDCF]) {
> > > + u32 tdcf = nla_get_u32(tb_tdc[IFLA_CAN_TDC_TDCF]);
> > > +
> > > + if (tdcf < tdc_const->tdcf_min || tdcf > tdc_const->tdcf_max)
> > > + return -EINVAL;
> > > +
> > > + tdc->tdcf = tdcf;
> > > + }
> > > +
> > > + return 0;
> > > +}
> >
> > To reproduce (ip pseudo-code only :D ):
> >
> > ip down
> > ip up tdc-mode manual tdco 111 tdcv 33 # 111 is out of range, 33 is valid
> > ip down
> > ip up # results in tdco=0 tdcv=33 mode=manual
>
> I do not think that this PoC would work because, thankfully, the
> netlink interface uses a mutex to prevent this issue from
> occurring.
It works, I've tested it :)
> That mutex is defined in:
> https://elixir.bootlin.com/linux/latest/source/net/core/rtnetlink.c#L68
>
> Each time a netlink message is sent to the kernel, it would be
> dispatched by rtnetlink_rcv_msg() which will make sure to lock
> the mutex before doing so:
> https://elixir.bootlin.com/linux/latest/source/net/core/rtnetlink.c#L5551
>
> A funny note is that because the mutex is global, if you run two
> ip command in a row:
>
> | ip link set can0 type can bitrate 500000
> | ip link set can1 up
>
> the second one will wait for the first one to finish even if it
> is on a different network device.
>
> To conclude, I do not think this needs to be fixed.
It's not a race. Consider this command:
| ip up tdc-mode manual tdco 111 tdcv 33 # 111 is out of range, 33 is valid
tdcv is checked first and valid, then it's assigned to the priv->tdc.
tdco is checked second and invalid, then can_tdc_changelink() returns -EINVAL.
tdc ends up being half set :(
So the setting of tdc is inconsistent and when you do a "ip down" "ip
up" then it results in a tdco=0 tdcv=33 mode=manual.
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 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2021-08-18 8:19 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-15 3:32 [PATCH v5 0/7] add the netlink interface for CAN-FD Transmitter Delay Compensation (TDC) Vincent Mailhol
2021-08-15 3:32 ` [PATCH v5 1/7] can: netlink: allow user to turn off unsupported features Vincent Mailhol
2021-08-19 7:45 ` Marc Kleine-Budde
2021-08-19 9:24 ` Vincent MAILHOL
2021-08-19 10:07 ` Marc Kleine-Budde
2021-08-15 3:32 ` [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min Vincent Mailhol
2021-08-16 8:42 ` Marc Kleine-Budde
2021-08-16 10:24 ` Vincent MAILHOL
2021-08-16 12:25 ` Marc Kleine-Budde
2021-08-16 12:33 ` Marc Kleine-Budde
2021-08-16 13:43 ` Marc Kleine-Budde
2021-08-16 15:49 ` Vincent MAILHOL
2021-08-16 16:04 ` Vincent MAILHOL
2021-08-17 1:12 ` Vincent MAILHOL
2021-08-17 20:01 ` Marc Kleine-Budde
2021-08-18 9:22 ` Vincent MAILHOL
2021-08-18 12:29 ` Marc Kleine-Budde
2021-08-18 14:17 ` Vincent MAILHOL
2021-08-20 6:12 ` Vincent MAILHOL
2021-08-16 14:10 ` Vincent MAILHOL
2021-08-16 14:30 ` Marc Kleine-Budde
2021-08-17 9:43 ` Marc Kleine-Budde
2021-08-16 13:49 ` Stefan Mätje
2021-08-16 15:56 ` Vincent MAILHOL
2021-08-15 3:32 ` [PATCH v5 3/7] can: bittiming: change unit of TDC parameters to clock periods Vincent Mailhol
2021-08-15 3:32 ` [PATCH v5 4/7] can: dev: add can_tdc_get_relative_tdco() helper function Vincent Mailhol
2021-08-16 11:27 ` Marc Kleine-Budde
2021-08-15 3:32 ` [PATCH v5 5/7] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC) Vincent Mailhol
2021-08-17 19:55 ` Marc Kleine-Budde
2021-08-18 8:08 ` Vincent MAILHOL
2021-08-18 8:19 ` Marc Kleine-Budde [this message]
2021-08-18 8:37 ` Vincent MAILHOL
2021-08-18 8:43 ` Marc Kleine-Budde
2021-08-15 3:32 ` [PATCH v5 6/7] can: netlink: add can_priv::do_get_auto_tdcv() to retrieve tdcv from device Vincent Mailhol
2021-08-15 3:32 ` [PATCH v5 7/7] can: etas_es58x: clean-up documentation of struct es58x_fd_tx_conf_msg Vincent Mailhol
2021-08-19 10:08 ` Marc Kleine-Budde
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210818081934.6f23ghoom2dkv53m@pengutronix.de \
--to=mkl@pengutronix.de \
--cc=Stefan.Maetje@esd.eu \
--cc=linux-can@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mailhol.vincent@wanadoo.fr \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).