| Message ID | 20240104111132.42730-5-arkadiusz.kubalewski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | dpll: fix unordered unbind/bind registerer issues | expand |
Thu, Jan 04, 2024 at 12:11:32PM CET, arkadiusz.kubalewski@intel.com wrote: >If parent pin was unregistered but child pin was not, the userspace >would see the "zombie" pins - the ones that were registered with >parent pin (pins_pin_on_pin_register(..)). >Technically those are not available - as there is no dpll device in the >system. Do not dump those pins and prevent userspace from any >interaction with them. Ah, here it is :) > >Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions") >Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions") >Reviewed-by: Jan Glaza <jan.glaza@intel.com> >Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >--- > drivers/dpll/dpll_netlink.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > >diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c >index f266db8da2f0..495dfc43c0be 100644 >--- a/drivers/dpll/dpll_netlink.c >+++ b/drivers/dpll/dpll_netlink.c >@@ -949,6 +949,19 @@ dpll_pin_parent_pin_set(struct dpll_pin *pin, struct nlattr *parent_nest, > return 0; > } > >+static bool dpll_pin_parents_registered(struct dpll_pin *pin) >+{ >+ struct dpll_pin_ref *par_ref; >+ struct dpll_pin *p; >+ unsigned long i, j; >+ >+ xa_for_each(&pin->parent_refs, i, par_ref) >+ xa_for_each_marked(&dpll_pin_xa, j, p, DPLL_REGISTERED) >+ if (par_ref->pin == p) >+ return true; if (xa_get_mark(..)) return true; ? >+ return false; As I wrote in the reply to the other patch, could you unify the "hide" behaviour for unregistered parent pin/device? >+} >+ > static int > dpll_pin_set_from_nlattr(struct dpll_pin *pin, struct genl_info *info) > { >@@ -1153,6 +1166,9 @@ int dpll_nl_pin_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb) > > xa_for_each_marked_start(&dpll_pin_xa, i, pin, DPLL_REGISTERED, > ctx->idx) { >+ if (!xa_empty(&pin->parent_refs) && This empty check is redundant, remove it. >+ !dpll_pin_parents_registered(pin)) >+ continue; > hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid, > cb->nlh->nlmsg_seq, > &dpll_nl_family, NLM_F_MULTI, >@@ -1179,6 +1195,10 @@ int dpll_nl_pin_set_doit(struct sk_buff *skb, struct genl_info *info) > { > struct dpll_pin *pin = info->user_ptr[0]; > >+ if (!xa_empty(&pin->parent_refs) && This empty check is redundant, remove it. >+ !dpll_pin_parents_registered(pin)) >+ return -ENODEV; >+ > return dpll_pin_set_from_nlattr(pin, info); > } > >-- >2.38.1 >
Thu, Jan 04, 2024 at 12:11:32PM CET, arkadiusz.kubalewski@intel.com wrote: [...] >@@ -1179,6 +1195,10 @@ int dpll_nl_pin_set_doit(struct sk_buff *skb, struct genl_info *info) What about dpll_nl_pin_get_doit(), dpll_nl_pin_id_get_doit()? I think it would be better to move the check to: dpll_pin_pre_doit() > { > struct dpll_pin *pin = info->user_ptr[0]; > >+ if (!xa_empty(&pin->parent_refs) && >+ !dpll_pin_parents_registered(pin)) >+ return -ENODEV; >+ > return dpll_pin_set_from_nlattr(pin, info); > } > >-- >2.38.1 >
Thu, Jan 04, 2024 at 12:11:32PM CET, arkadiusz.kubalewski@intel.com wrote: [...] >@@ -1179,6 +1195,10 @@ int dpll_nl_pin_set_doit(struct sk_buff *skb, struct genl_info *info) > { > struct dpll_pin *pin = info->user_ptr[0]; > >+ if (!xa_empty(&pin->parent_refs) && >+ !dpll_pin_parents_registered(pin)) >+ return -ENODEV; >+ Also make sure to prevent events from being send for this pin. > return dpll_pin_set_from_nlattr(pin, info); > } > >-- >2.38.1 >
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Thursday, January 4, 2024 4:09 PM > >Thu, Jan 04, 2024 at 12:11:32PM CET, arkadiusz.kubalewski@intel.com wrote: > >[...] > >>@@ -1179,6 +1195,10 @@ int dpll_nl_pin_set_doit(struct sk_buff *skb, >>struct genl_info *info) >> { >> struct dpll_pin *pin = info->user_ptr[0]; >> >>+ if (!xa_empty(&pin->parent_refs) && >>+ !dpll_pin_parents_registered(pin)) >>+ return -ENODEV; >>+ > >Also make sure to prevent events from being send for this pin. > Sure, will do. Thank you! Arkadiusz > >> return dpll_pin_set_from_nlattr(pin, info); >> } >> >>-- >>2.38.1 >>
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Thursday, January 4, 2024 4:07 PM > >Thu, Jan 04, 2024 at 12:11:32PM CET, arkadiusz.kubalewski@intel.com wrote: > >[...] > > >>@@ -1179,6 +1195,10 @@ int dpll_nl_pin_set_doit(struct sk_buff *skb, >struct genl_info *info) > > >What about dpll_nl_pin_get_doit(), dpll_nl_pin_id_get_doit()? > >I think it would be better to move the check to: >dpll_pin_pre_doit() > Yes, makes sense, will move it to dpll_pin_pre_doit(). Then won't be needed in dpll_nl_pin_get_doit() and dpll_nl_pin_set_doit(). Plus, will add check in dpll_nl_pin_id_get_doit(). Thank you! Arkadiusz > >> { >> struct dpll_pin *pin = info->user_ptr[0]; >> >>+ if (!xa_empty(&pin->parent_refs) && >>+ !dpll_pin_parents_registered(pin)) >>+ return -ENODEV; >>+ >> return dpll_pin_set_from_nlattr(pin, info); >> } >> >>-- >>2.38.1 >>
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Thursday, January 4, 2024 4:05 PM >To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com> >Cc: netdev@vger.kernel.org; vadim.fedorenko@linux.dev; >michal.michalik@intel.com; Olech, Milena <milena.olech@intel.com>; >pabeni@redhat.com; kuba@kernel.org; Glaza, Jan <jan.glaza@intel.com> >Subject: Re: [PATCH net v2 4/4] dpll: hide "zombie" pins for userspace > >Thu, Jan 04, 2024 at 12:11:32PM CET, arkadiusz.kubalewski@intel.com wrote: >>If parent pin was unregistered but child pin was not, the userspace >>would see the "zombie" pins - the ones that were registered with >>parent pin (pins_pin_on_pin_register(..)). >>Technically those are not available - as there is no dpll device in the >>system. Do not dump those pins and prevent userspace from any >>interaction with them. > >Ah, here it is :) > :) > >> >>Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions") >>Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions") >>Reviewed-by: Jan Glaza <jan.glaza@intel.com> >>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>--- >> drivers/dpll/dpll_netlink.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c >>index f266db8da2f0..495dfc43c0be 100644 >>--- a/drivers/dpll/dpll_netlink.c >>+++ b/drivers/dpll/dpll_netlink.c >>@@ -949,6 +949,19 @@ dpll_pin_parent_pin_set(struct dpll_pin *pin, struct >nlattr *parent_nest, >> return 0; >> } >> >>+static bool dpll_pin_parents_registered(struct dpll_pin *pin) >>+{ >>+ struct dpll_pin_ref *par_ref; >>+ struct dpll_pin *p; >>+ unsigned long i, j; >>+ >>+ xa_for_each(&pin->parent_refs, i, par_ref) >>+ xa_for_each_marked(&dpll_pin_xa, j, p, DPLL_REGISTERED) >>+ if (par_ref->pin == p) >>+ return true; > > if (xa_get_mark(..)) > return true; >? > Sure, will do. > >>+ return false; > > >As I wrote in the reply to the other patch, could you unify the "hide" >behaviour for unregistered parent pin/device? > Yes, in v3. > >>+} >>+ >> static int >> dpll_pin_set_from_nlattr(struct dpll_pin *pin, struct genl_info *info) >> { >>@@ -1153,6 +1166,9 @@ int dpll_nl_pin_get_dumpit(struct sk_buff *skb, >>struct netlink_callback *cb) >> >> xa_for_each_marked_start(&dpll_pin_xa, i, pin, DPLL_REGISTERED, >> ctx->idx) { >>+ if (!xa_empty(&pin->parent_refs) && > >This empty check is redundant, remove it. > Ok. > >>+ !dpll_pin_parents_registered(pin)) >>+ continue; >> hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid, >> cb->nlh->nlmsg_seq, >> &dpll_nl_family, NLM_F_MULTI, >>@@ -1179,6 +1195,10 @@ int dpll_nl_pin_set_doit(struct sk_buff *skb, >>struct genl_info *info) >> { >> struct dpll_pin *pin = info->user_ptr[0]; >> >>+ if (!xa_empty(&pin->parent_refs) && > >This empty check is redundant, remove it. > Will do. Thank you! Arkadiusz > >>+ !dpll_pin_parents_registered(pin)) >>+ return -ENODEV; >>+ >> return dpll_pin_set_from_nlattr(pin, info); >> } >> >>-- >>2.38.1 >>
diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c index f266db8da2f0..495dfc43c0be 100644 --- a/drivers/dpll/dpll_netlink.c +++ b/drivers/dpll/dpll_netlink.c @@ -949,6 +949,19 @@ dpll_pin_parent_pin_set(struct dpll_pin *pin, struct nlattr *parent_nest, return 0; } +static bool dpll_pin_parents_registered(struct dpll_pin *pin) +{ + struct dpll_pin_ref *par_ref; + struct dpll_pin *p; + unsigned long i, j; + + xa_for_each(&pin->parent_refs, i, par_ref) + xa_for_each_marked(&dpll_pin_xa, j, p, DPLL_REGISTERED) + if (par_ref->pin == p) + return true; + return false; +} + static int dpll_pin_set_from_nlattr(struct dpll_pin *pin, struct genl_info *info) { @@ -1153,6 +1166,9 @@ int dpll_nl_pin_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb) xa_for_each_marked_start(&dpll_pin_xa, i, pin, DPLL_REGISTERED, ctx->idx) { + if (!xa_empty(&pin->parent_refs) && + !dpll_pin_parents_registered(pin)) + continue; hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq, &dpll_nl_family, NLM_F_MULTI, @@ -1179,6 +1195,10 @@ int dpll_nl_pin_set_doit(struct sk_buff *skb, struct genl_info *info) { struct dpll_pin *pin = info->user_ptr[0]; + if (!xa_empty(&pin->parent_refs) && + !dpll_pin_parents_registered(pin)) + return -ENODEV; + return dpll_pin_set_from_nlattr(pin, info); }
If parent pin was unregistered but child pin was not, the userspace would see the "zombie" pins - the ones that were registered with parent pin (pins_pin_on_pin_register(..)). Technically those are not available - as there is no dpll device in the system. Do not dump those pins and prevent userspace from any interaction with them. Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions") Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions") Reviewed-by: Jan Glaza <jan.glaza@intel.com> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> --- drivers/dpll/dpll_netlink.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)