Re: [PATCH net] dpll: rely on rcu for netdev_dpll_pin()

[Jiri Pirko <jiri@resnulli.us>]

Fri, Feb 23, 2024 at 12:51:43PM CET, edumazet@google.com wrote:
>On Fri, Feb 23, 2024 at 10:00 AM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Thu, Feb 22, 2024 at 05:48:51PM CET, edumazet@google.com wrote:
>> >This fixes a possible UAF in if_nlmsg_size(),
>> >which can run without RTNL.
>> >
>> >Add rcu protection to "struct dpll_pin"
>> >
>> >Note: This looks possible to no longer acquire RTNL in
>> >netdev_dpll_pin_assign().
>>
>> Yeah, looks like no longer needed. Will you do a follow-up for net-next
>> once this is applied to -net?
>
>Absolutely, this came while I was working on my RTNL->RCU conversion
>of "ip l" in net-next.
>
>
>
>>
>>
>> >
>> >Fixes: 5f1842692880 ("netdev: expose DPLL pin handle for netdevice")
>> >Signed-off-by: Eric Dumazet <edumazet@google.com>
>> >Cc: Jiri Pirko <jiri@nvidia.com>
>> >Cc: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>> >Cc: Vadim Fedorenko <vadim.fedorenko@linux.dev>
>> >---
>> > drivers/dpll/dpll_core.c  |  2 +-
>> > drivers/dpll/dpll_core.h  |  2 ++
>> > include/linux/dpll.h      | 11 +++++++++++
>> > include/linux/netdevice.h | 11 +----------
>> > net/core/dev.c            |  2 +-
>> > net/core/rtnetlink.c      |  2 ++
>> > 6 files changed, 18 insertions(+), 12 deletions(-)
>> >
>> >diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>> >index 5152bd1b0daf599869195e81805fbb2709dbe6b4..4c2bb27c99fe4e517b0d92c4ae3db83a679c7d11 100644
>> >--- a/drivers/dpll/dpll_core.c
>> >+++ b/drivers/dpll/dpll_core.c
>> >@@ -564,7 +564,7 @@ void dpll_pin_put(struct dpll_pin *pin)
>> >               xa_destroy(&pin->parent_refs);
>> >               xa_erase(&dpll_pin_xa, pin->id);
>> >               dpll_pin_prop_free(&pin->prop);
>> >-              kfree(pin);
>> >+              kfree_rcu(pin, rcu);
>> >       }
>> >       mutex_unlock(&dpll_lock);
>> > }
>> >diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h
>> >index 717f715015c742238d5585fddc5cd267fbb0db9f..2b6d8ef1cdf36cff24328e497c49d667659dd0e6 100644
>> >--- a/drivers/dpll/dpll_core.h
>> >+++ b/drivers/dpll/dpll_core.h
>> >@@ -47,6 +47,7 @@ struct dpll_device {
>> >  * @prop:             pin properties copied from the registerer
>> >  * @rclk_dev_name:    holds name of device when pin can recover clock from it
>> >  * @refcount:         refcount
>> >+ * @rcu:              rcu_head for kfree_rcu()
>> >  **/
>> > struct dpll_pin {
>> >       u32 id;
>> >@@ -57,6 +58,7 @@ struct dpll_pin {
>> >       struct xarray parent_refs;
>> >       struct dpll_pin_properties prop;
>> >       refcount_t refcount;
>> >+      struct rcu_head rcu;
>> > };
>> >
>> > /**
>> >diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>> >index 9cf896ea1d4122f3bc7094e46a5af81b999937dc..4ec2fe9caf5a3f284afd0cfe4fc7c2bf42cbbc60 100644
>> >--- a/include/linux/dpll.h
>> >+++ b/include/linux/dpll.h
>> >@@ -10,6 +10,8 @@
>> > #include <uapi/linux/dpll.h>
>> > #include <linux/device.h>
>> > #include <linux/netlink.h>
>> >+#include <linux/netdevice.h>
>> >+#include <linux/rtnetlink.h>
>> >
>> > struct dpll_device;
>> > struct dpll_pin;
>> >@@ -167,4 +169,13 @@ int dpll_device_change_ntf(struct dpll_device *dpll);
>> >
>> > int dpll_pin_change_ntf(struct dpll_pin *pin);
>> >
>> >+static inline struct dpll_pin *netdev_dpll_pin(const struct net_device *dev)
>> >+{
>> >+#if IS_ENABLED(CONFIG_DPLL)
>> >+      return rcu_dereference_rtnl(dev->dpll_pin);
>> >+#else
>> >+      return NULL;
>> >+#endif
>>
>> Why you moved netdev_dpll_pin() here?
>
>Because of the rcu_dereference_rtnl() call, not available to
>include/linux/netdevice.h
>
>Adding the missing include would have been more painful.
>include/linux/netdevice.h being included everywhere and being bloated already,
>I think moving netdev_dpll_pin() helper in  dpll.h is better (should
>not increase compile time for the kernel)

Fair enough.


>
>>
>>
>> >+}
>> >+
>> > #endif
>> >diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> >index ef7bfbb9849733fa7f1f097ba53a36a68cc3384b..a9c973b92294bb110cf3cd336485972127b01b58 100644
>> >--- a/include/linux/netdevice.h
>> >+++ b/include/linux/netdevice.h
>> >@@ -2469,7 +2469,7 @@ struct net_device {
>> >       struct devlink_port     *devlink_port;
>> >
>> > #if IS_ENABLED(CONFIG_DPLL)
>> >-      struct dpll_pin         *dpll_pin;
>> >+      struct dpll_pin __rcu   *dpll_pin;
>> > #endif
>> > #if IS_ENABLED(CONFIG_PAGE_POOL)
>> >       /** @page_pools: page pools created for this netdevice */
>> >@@ -4035,15 +4035,6 @@ bool netdev_port_same_parent_id(struct net_device *a, struct net_device *b);
>> > void netdev_dpll_pin_set(struct net_device *dev, struct dpll_pin *dpll_pin);
>> > void netdev_dpll_pin_clear(struct net_device *dev);
>> >
>> >-static inline struct dpll_pin *netdev_dpll_pin(const struct net_device *dev)
>> >-{
>> >-#if IS_ENABLED(CONFIG_DPLL)
>> >-      return dev->dpll_pin;
>> >-#else
>> >-      return NULL;
>> >-#endif
>> >-}
>> >-
>> > struct sk_buff *validate_xmit_skb_list(struct sk_buff *skb, struct net_device *dev, bool *again);
>> > struct sk_buff *dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
>> >                                   struct netdev_queue *txq, int *ret);
>> >diff --git a/net/core/dev.c b/net/core/dev.c
>> >index 73a0219730075e666c4f11f668a50dbf9f9afa97..0230391c78f71e22d3c0e925ff8d3d792aa54a32 100644
>> >--- a/net/core/dev.c
>> >+++ b/net/core/dev.c
>> >@@ -9078,7 +9078,7 @@ static void netdev_dpll_pin_assign(struct net_device *dev, struct dpll_pin *dpll
>> > {
>> > #if IS_ENABLED(CONFIG_DPLL)
>> >       rtnl_lock();
>> >-      dev->dpll_pin = dpll_pin;
>> >+      rcu_assign_pointer(dev->dpll_pin, dpll_pin);
>> >       rtnl_unlock();
>> > #endif
>> > }
>> >diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> >index 9c4f427f3a5057b52ec05405e8b15b8ca2246b4b..6239aa62a29cb8752a53e3f75a48a1e2fdd3b0ec 100644
>> >--- a/net/core/rtnetlink.c
>> >+++ b/net/core/rtnetlink.c
>> >@@ -1057,7 +1057,9 @@ static size_t rtnl_dpll_pin_size(const struct net_device *dev)
>> > {
>> >       size_t size = nla_total_size(0); /* nest IFLA_DPLL_PIN */
>> >
>> >+      rcu_read_lock();
>>
>> Why do you need to take the rcu read lock here? Isn't this called
>> with either rtnl held of rcu read lock held?
>>
>> And if you need to take rcu read lock here, why you use
>> rcu_dereference_rtnl() instead of rcu_dereference() in
>> netdev_dpll_pin()?
>
>This is because I discovered the issue while working on net-next tree,
>and basically had to fix the bug in net tree, I had to prune my WIP to
>get to the fix.
>
>You are right, the rcu_read_lock() is not really needed here.

Okay.