Re: [PATCH] ipv6: mcast: Add __must_hold() annotations.

[Brent Pappas <bpappas@pappasbrent.com>]

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