| Message ID | 20240808190256.149602-1-bpappas@pappasbrent.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ipv6: mcast: Add __must_hold() annotations. | expand |
From: Brent Pappas <bpappas@pappasbrent.com> Date: Thu, 8 Aug 2024 15:02:55 -0400 > Add __must_hold(RCU) annotations to igmp6_mc_get_first(), > igmp6_mc_get_next(), and igmp6_mc_get_idx() to signify that they are > meant to be called in RCU critical sections. > > Signed-off-by: Brent Pappas <bpappas@pappasbrent.com> > --- > net/ipv6/mcast.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c > index 7ba01d8cfbae..843d0d065242 100644 > --- a/net/ipv6/mcast.c > +++ b/net/ipv6/mcast.c > @@ -22,6 +22,7 @@ > * - MLDv2 support > */ > > +#include "linux/compiler_types.h" Why "" ? Btw, I think for_each_netdev_rcu() / rcu_dereference() in touched functions are enough to cleary annotate RCU is needed there. Even without it, I prefer rcu_read_lock_held(), I'm not sure to what extent sparse can analyse functions statically though. > #include <linux/module.h> > #include <linux/errno.h> > #include <linux/types.h> > @@ -2861,6 +2862,7 @@ struct igmp6_mc_iter_state { > #define igmp6_mc_seq_private(seq) ((struct igmp6_mc_iter_state *)(seq)->private) > > static inline struct ifmcaddr6 *igmp6_mc_get_first(struct seq_file *seq) > + __must_hold(RCU) > { > struct ifmcaddr6 *im = NULL; > struct igmp6_mc_iter_state *state = igmp6_mc_seq_private(seq); > @@ -2882,7 +2884,9 @@ static inline struct ifmcaddr6 *igmp6_mc_get_first(struct seq_file *seq) > return im; > } > > -static struct ifmcaddr6 *igmp6_mc_get_next(struct seq_file *seq, struct ifmcaddr6 *im) > +static struct ifmcaddr6 *igmp6_mc_get_next(struct seq_file *seq, > + struct ifmcaddr6 *im) > + __must_hold(RCU) > { > struct igmp6_mc_iter_state *state = igmp6_mc_seq_private(seq); > > @@ -2902,6 +2906,7 @@ static struct ifmcaddr6 *igmp6_mc_get_next(struct seq_file *seq, struct ifmcaddr > } > > static struct ifmcaddr6 *igmp6_mc_get_idx(struct seq_file *seq, loff_t pos) > + __must_hold(RCU) > { > struct ifmcaddr6 *im = igmp6_mc_get_first(seq); > if (im) > -- > 2.46.0
The 08/08/2024 13:23, Kuniyuki Iwashima wrote: > From: Brent Pappas <bpappas@pappasbrent.com> > Date: Thu, 8 Aug 2024 15:02:55 -0400 > > Add __must_hold(RCU) annotations to igmp6_mc_get_first(), > > igmp6_mc_get_next(), and igmp6_mc_get_idx() to signify that they are > > meant to be called in RCU critical sections. > > > > Signed-off-by: Brent Pappas <bpappas@pappasbrent.com> > > --- > > net/ipv6/mcast.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c > > index 7ba01d8cfbae..843d0d065242 100644 > > --- a/net/ipv6/mcast.c > > +++ b/net/ipv6/mcast.c > > @@ -22,6 +22,7 @@ > > * - MLDv2 support > > */ > > > > +#include "linux/compiler_types.h" > Why "" ? That was an accident; my language server (clangd) inserted the include for me while I was typing and included the header with quotes instead of angle brackets. I will submit a corrected patch if that will make this change acceptable. > Btw, I think for_each_netdev_rcu() / rcu_dereference() in touched > functions are enough to cleary annotate RCU is needed there. I see your point. I noticed that igmp6_mc_seq_start() calls rcu_read_lock() and is annotated with __acquires(), and igmp6_mc_seq_stop() calls rcu_read_unlock() and is annotated with __releases(), so it seemed to me that the extra __must_hold() annotations would be preferable. Unless there's a reason to prefer __acquires() and __releases() over __must_hold()? > Even without it, I prefer rcu_read_lock_held(), I'm not sure to > what extent sparse can analyse functions statically though. AFAIK, Sparse only uses these annotations to check for context imbalances, and does not check, e.g., whether macros that access shared values such as rcu_dereference() are only invoked in critical sections. Full disclosure, I am working on a static analysis tool called Macroni to provide more static checks for RCU (this is how I found these unannotated functions). > > #include <linux/module.h> > > #include <linux/errno.h> > > #include <linux/types.h> > > @@ -2861,6 +2862,7 @@ struct igmp6_mc_iter_state { > > #define igmp6_mc_seq_private(seq) ((struct igmp6_mc_iter_state *)(seq)->private) > > > > static inline struct ifmcaddr6 *igmp6_mc_get_first(struct seq_file *seq) > > + __must_hold(RCU) > > { > > struct ifmcaddr6 *im = NULL; > > struct igmp6_mc_iter_state *state = igmp6_mc_seq_private(seq); > > @@ -2882,7 +2884,9 @@ static inline struct ifmcaddr6 *igmp6_mc_get_first(struct seq_file *seq) > > return im; > > } > > > > -static struct ifmcaddr6 *igmp6_mc_get_next(struct seq_file *seq, struct ifmcaddr6 *im) > > +static struct ifmcaddr6 *igmp6_mc_get_next(struct seq_file *seq, > > + struct ifmcaddr6 *im) > > + __must_hold(RCU) > > { > > struct igmp6_mc_iter_state *state = igmp6_mc_seq_private(seq); > > > > @@ -2902,6 +2906,7 @@ static struct ifmcaddr6 *igmp6_mc_get_next(struct seq_file *seq, struct ifmcaddr > > } > > > > static struct ifmcaddr6 *igmp6_mc_get_idx(struct seq_file *seq, loff_t pos) > > + __must_hold(RCU) > > { > > struct ifmcaddr6 *im = igmp6_mc_get_first(seq); > > if (im) > > -- > > 2.46.0
diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c index 7ba01d8cfbae..843d0d065242 100644 --- a/net/ipv6/mcast.c +++ b/net/ipv6/mcast.c @@ -22,6 +22,7 @@ * - MLDv2 support */ +#include "linux/compiler_types.h" #include <linux/module.h> #include <linux/errno.h> #include <linux/types.h> @@ -2861,6 +2862,7 @@ struct igmp6_mc_iter_state { #define igmp6_mc_seq_private(seq) ((struct igmp6_mc_iter_state *)(seq)->private) static inline struct ifmcaddr6 *igmp6_mc_get_first(struct seq_file *seq) + __must_hold(RCU) { struct ifmcaddr6 *im = NULL; struct igmp6_mc_iter_state *state = igmp6_mc_seq_private(seq); @@ -2882,7 +2884,9 @@ static inline struct ifmcaddr6 *igmp6_mc_get_first(struct seq_file *seq) return im; } -static struct ifmcaddr6 *igmp6_mc_get_next(struct seq_file *seq, struct ifmcaddr6 *im) +static struct ifmcaddr6 *igmp6_mc_get_next(struct seq_file *seq, + struct ifmcaddr6 *im) + __must_hold(RCU) { struct igmp6_mc_iter_state *state = igmp6_mc_seq_private(seq); @@ -2902,6 +2906,7 @@ static struct ifmcaddr6 *igmp6_mc_get_next(struct seq_file *seq, struct ifmcaddr } static struct ifmcaddr6 *igmp6_mc_get_idx(struct seq_file *seq, loff_t pos) + __must_hold(RCU) { struct ifmcaddr6 *im = igmp6_mc_get_first(seq); if (im)
Add __must_hold(RCU) annotations to igmp6_mc_get_first(), igmp6_mc_get_next(), and igmp6_mc_get_idx() to signify that they are meant to be called in RCU critical sections. Signed-off-by: Brent Pappas <bpappas@pappasbrent.com> --- net/ipv6/mcast.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)