Received: by 2002:a05:6a10:1d13:0:0:0:0 with SMTP id pp19csp250507pxb; Wed, 18 Aug 2021 01:11:38 -0700 (PDT) X-Google-Smtp-Source: ABdhPJz0150HpeJvXJHmCB5YoczpsiyIRdOox/sfYLomu6vxDnZxrSq3NWv2cDElJpvL5zqz8YRY X-Received: by 2002:a05:6e02:1288:: with SMTP id y8mr5226578ilq.206.1629274298603; Wed, 18 Aug 2021 01:11:38 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1629274298; cv=none; d=google.com; s=arc-20160816; b=yrLdsByWtn92c9qnEp4rk02AYI7nNxa4jJdgsu7ho9fW+HgQGf+4Td/qS6Vet2W4UN 3qItLErn/j/u4gx0Zj2Pj75biOvJfqbgDtheW5R/FZ3rXV/frbaCehKhFmclCIH2QJoC RPQGsEtHNOC/zIVb5XerWUVw/OynJq4tn3emNqcEfHT9EAQaxOMp/rQ1Ydo5N5PzBON2 FCNkTT+PvDmna2YvPe43kyF+3iRO0ex20Axor7YRgpmIg7soahS8mQYpv/64VvWI+CQ4 wVuqxyShTaVm/jUiTNcfU+S9ZcVq3LxkW1nm0CrRXxKW88BMUKscv0WVO6GN832961ni 013Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version; bh=zS3HmGadTV/ONEUOpne6IQWm4IBd42AxFALL1jwUdAk=; b=k+1oQivdkmWa634qgrrOcnVmInK4GusvQTKFvSFwfce0BDjRzrmZSFNxKWRMi2+lkZ mpAIl6rc1BmBmytWm3O8n4D7vpvOQ9z4n1IbFmMg1ZPev24O8wA8bz9McNnqu5i6iekI HKpEBVIoCd/9Pc/8hw+FaJqkc700fYMkb6vwp4OGaBsCGehz/LAkChT8YSzzzFpfVNb0 odGW2tCLbBb1anXerZxshKfAT9y8CvfyrFJFBwhFRGrbjh+zAH+S/tQLDsgjMZfZ50yh 6oU9dGl/5Sdv1DFxtHVNMxQZNlkCqoquy12UFM9iUUnrS78yL5b9EcGFlqTGnGYk5EVk tq3g== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id j4si4629129ilc.2.2021.08.18.01.11.26; Wed, 18 Aug 2021 01:11:38 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239519AbhHRIJj (ORCPT + 99 others); Wed, 18 Aug 2021 04:09:39 -0400 Received: from mail-lj1-f172.google.com ([209.85.208.172]:33561 "EHLO mail-lj1-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238650AbhHRIJi (ORCPT ); Wed, 18 Aug 2021 04:09:38 -0400 Received: by mail-lj1-f172.google.com with SMTP id n7so3629744ljq.0; Wed, 18 Aug 2021 01:09:03 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=zS3HmGadTV/ONEUOpne6IQWm4IBd42AxFALL1jwUdAk=; b=bpsTizG81nzqZZb4mMT4MYy+MNPh3DyB65e7F9Yag5xXP1ql5xzPwdWWMBp+rdHW3N 1Ez3BrKMH+iupIseDou1kapDEGXbdU2qd4FPMxnFosg8GbBwDhMN6un3NXiJJKAXXXcr IBPfk5deMBv36hj006pKtxUkehpwPu6OR3kzIsb+X8m8hp4zEhch3CiCbb9FULSwxSbu K9aVnTxqNWxWG1nG94T3nF+JNfJumdc8hT1E3jXYozYDR5K57C+IXj7HZcre0/apcdta 8E+462Vw1YBn5fVRL3RkO7mqaZTupCUhpkHPkKqfWioZoMdulDiq+DAJavHNtoKYlN7m OeLg== X-Gm-Message-State: AOAM530DxnTinbkStZ3xRgQSgV+ZGRlK9sHz33bTQkcD5IVVX0uAV5MQ u7p+r3NJTUNUWXFftsIgd57H+u1NFd0BlSR4IVw= X-Received: by 2002:a2e:9c08:: with SMTP id s8mr6828658lji.331.1629274142982; Wed, 18 Aug 2021 01:09:02 -0700 (PDT) MIME-Version: 1.0 References: <20210815033248.98111-1-mailhol.vincent@wanadoo.fr> <20210815033248.98111-6-mailhol.vincent@wanadoo.fr> <20210817195551.wwgu7dnhb6qyvo7n@pengutronix.de> In-Reply-To: <20210817195551.wwgu7dnhb6qyvo7n@pengutronix.de> From: Vincent MAILHOL Date: Wed, 18 Aug 2021 17:08:51 +0900 Message-ID: Subject: Re: [PATCH v5 5/7] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC) To: Marc Kleine-Budde Cc: linux-can , =?UTF-8?Q?Stefan_M=C3=A4tje?= , netdev , open list Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed 18 Aug 2021 at 04:55, Marc Kleine-Budde 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. > > + } > > + > > + 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. 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. Yours sincerely, Vincent