| Message ID | 20240503192059.3884225-4-edumazet@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ad13b5b0d1f9eb8e048394919e6393e520b14552 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | rtnetlink: more rcu conversions for rtnl_fill_ifinfo() | expand |
On Fri, May 03, 2024 at 07:20:54PM +0000, Eric Dumazet wrote: > rtnl_fill_ifinfo() can read dev->tx_queue_len locklessly, > granted we add corresponding READ_ONCE()/WRITE_ONCE() annotations. > > Add missing READ_ONCE(dev->tx_queue_len) in teql_enqueue() Hi Eric, I am wondering if READ_ONCE(caifd->netdev->tx_queue_len) is also missing from net/caif/caif_dev.c:transmit(). ...
On Sun, 2024-05-05 at 15:43 +0100, Simon Horman wrote: > On Fri, May 03, 2024 at 07:20:54PM +0000, Eric Dumazet wrote: > > rtnl_fill_ifinfo() can read dev->tx_queue_len locklessly, > > granted we add corresponding READ_ONCE()/WRITE_ONCE() annotations. > > > > Add missing READ_ONCE(dev->tx_queue_len) in teql_enqueue() > > Hi Eric, > > I am wondering if READ_ONCE(caifd->netdev->tx_queue_len) > is also missing from net/caif/caif_dev.c:transmit(). I agree such read is outside the rtnl lock and could use a READ_ONCE annotation. I think it's better to handle that as an eventual follow-up instead of blocking this series. Thanks, Paolo
On Tue, May 7, 2024 at 11:26 AM Paolo Abeni <pabeni@redhat.com> wrote: > > On Sun, 2024-05-05 at 15:43 +0100, Simon Horman wrote: > > On Fri, May 03, 2024 at 07:20:54PM +0000, Eric Dumazet wrote: > > > rtnl_fill_ifinfo() can read dev->tx_queue_len locklessly, > > > granted we add corresponding READ_ONCE()/WRITE_ONCE() annotations. > > > > > > Add missing READ_ONCE(dev->tx_queue_len) in teql_enqueue() > > > > Hi Eric, > > > > I am wondering if READ_ONCE(caifd->netdev->tx_queue_len) > > is also missing from net/caif/caif_dev.c:transmit(). > > I agree such read is outside the rtnl lock and could use a READ_ONCE > annotation. I think it's better to handle that as an eventual follow-up > instead of blocking this series. I missed Simon feedback, sorry. Yes, we can add missing READ_ONCE() as follow ups. They are all orthogonal.
On Tue, May 07, 2024 at 11:26:10AM +0200, Paolo Abeni wrote: > On Sun, 2024-05-05 at 15:43 +0100, Simon Horman wrote: > > On Fri, May 03, 2024 at 07:20:54PM +0000, Eric Dumazet wrote: > > > rtnl_fill_ifinfo() can read dev->tx_queue_len locklessly, > > > granted we add corresponding READ_ONCE()/WRITE_ONCE() annotations. > > > > > > Add missing READ_ONCE(dev->tx_queue_len) in teql_enqueue() > > > > Hi Eric, > > > > I am wondering if READ_ONCE(caifd->netdev->tx_queue_len) > > is also missing from net/caif/caif_dev.c:transmit(). > > I agree such read is outside the rtnl lock and could use a READ_ONCE > annotation. I think it's better to handle that as an eventual follow-up > instead of blocking this series. Yes, that is fine by me too. And I am happy to add it to my TODO list if no one else wants to take it. Reviewed-by: Simon Horman <horms@kernel.org>
diff --git a/net/core/dev.c b/net/core/dev.c index e02d2363347e2e403ccb2a59d44d35cee9a1b367..9c8c2ab2d76c3587d9114bc86a395341e1fd4d2b 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -8959,7 +8959,7 @@ int dev_change_tx_queue_len(struct net_device *dev, unsigned long new_len) return -ERANGE; if (new_len != orig_len) { - dev->tx_queue_len = new_len; + WRITE_ONCE(dev->tx_queue_len, new_len); res = call_netdevice_notifiers(NETDEV_CHANGE_TX_QUEUE_LEN, dev); res = notifier_to_errno(res); if (res) @@ -8973,7 +8973,7 @@ int dev_change_tx_queue_len(struct net_device *dev, unsigned long new_len) err_rollback: netdev_err(dev, "refused to change device tx_queue_len\n"); - dev->tx_queue_len = orig_len; + WRITE_ONCE(dev->tx_queue_len, orig_len); return res; } diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index a92e3b533d8d2ed1a52a40e02eb994c3070ede38..77d14528bdefc8b655f5da37ed88d0b937f35a61 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -1837,7 +1837,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, if (nla_put_string(skb, IFLA_IFNAME, devname)) goto nla_put_failure; - if (nla_put_u32(skb, IFLA_TXQLEN, dev->tx_queue_len) || + if (nla_put_u32(skb, IFLA_TXQLEN, READ_ONCE(dev->tx_queue_len)) || nla_put_u8(skb, IFLA_OPERSTATE, netif_running(dev) ? dev->operstate : IF_OPER_DOWN) || nla_put_u8(skb, IFLA_LINKMODE, dev->link_mode) || diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index 6292d6d73b720fef6766d08ed01d8b93a99f97b6..74afc210527d237cca3b48166be5918f802eb326 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -1334,7 +1334,7 @@ static struct Qdisc *qdisc_create(struct net_device *dev, * before again attaching a qdisc. */ if ((dev->priv_flags & IFF_NO_QUEUE) && (dev->tx_queue_len == 0)) { - dev->tx_queue_len = DEFAULT_TX_QUEUE_LEN; + WRITE_ONCE(dev->tx_queue_len, DEFAULT_TX_QUEUE_LEN); netdev_info(dev, "Caught tx_queue_len zero misconfig\n"); } diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c index 59304611dc0050e525de5f45b2a3b8628b684ff3..29850d0f073308290ac1a479bc98315034990663 100644 --- a/net/sched/sch_teql.c +++ b/net/sched/sch_teql.c @@ -78,7 +78,7 @@ teql_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free) struct net_device *dev = qdisc_dev(sch); struct teql_sched_data *q = qdisc_priv(sch); - if (q->q.qlen < dev->tx_queue_len) { + if (q->q.qlen < READ_ONCE(dev->tx_queue_len)) { __skb_queue_tail(&q->q, skb); return NET_XMIT_SUCCESS; }
rtnl_fill_ifinfo() can read dev->tx_queue_len locklessly, granted we add corresponding READ_ONCE()/WRITE_ONCE() annotations. Add missing READ_ONCE(dev->tx_queue_len) in teql_enqueue() Signed-off-by: Eric Dumazet <edumazet@google.com> --- net/core/dev.c | 4 ++-- net/core/rtnetlink.c | 2 +- net/sched/sch_api.c | 2 +- net/sched/sch_teql.c | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-)