Re: [PATCH 1/4] genirq/irq_sim: dispose of remaining mappings before removing the domain

[Yury Norov]

On Tue, Aug 15, 2023 at 01:38:49PM +0300, Andy Shevchenko wrote:
> On Sat, Aug 12, 2023 at 09:44:54PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > 
> > If the device providing simulated interrupts is unbound (real life
> > example: gpio-sim is disabled with users that didn't free their irqs)
> > and removes the simulated domain while interrupts are still requested,
> > we will hit memory issues when they are eventually freed and the
> > mappings destroyed in the process.
> > 
> > Specifically we'll access freed memory in __irq_domain_deactivate_irq().
> > 
> > Dispose of all mappings before removing the simulator domain.
> 
> ...
> 
> > +#include <linux/list.h>
> 
> Maybe ordered?
> 
> >  #include <linux/irq.h>
> >  #include <linux/irq_sim.h>
> >  #include <linux/irq_work.h>
> 
> ...
> 
> > @@ -16,12 +17,14 @@ struct irq_sim_work_ctx {
> >  	unsigned int		irq_count;
> >  	unsigned long		*pending;
> >  	struct irq_domain	*domain;
> > +	struct list_head	irqs;
> >  };
> >  
> >  struct irq_sim_irq_ctx {
> >  	int			irqnum;
> >  	bool			enabled;
> >  	struct irq_sim_work_ctx	*work_ctx;
> 
> > +	struct list_head	siblings;
> 
> You can reduce the code size by moving this to be the first member.
> Not sure about struct irq_sim_work_ctx, you can play with bloat-o-meter.

Pahole you meant?

  yury:linux$ pahole -C irq_sim_irq_ctx /sys/kernel/btf/vmlinux
  struct irq_sim_irq_ctx {
  	int                        irqnum;               /*     0     4 */
  	bool                       enabled;              /*     4     1 */
  
  	/* XXX 3 bytes hole, try to pack */
  
  	struct irq_sim_work_ctx *  work_ctx;             /*     8     8 */
  
  	/* size: 16, cachelines: 1, members: 3 */
  	/* sum members: 13, holes: 1, sum holes: 3 */
  	/* last cacheline: 16 bytes */
  };

In this particular case, there will be no hole because list head
position (16) will be aligned to sizeof(struct list_head) == 16.

But as Bartosz said in the other email, "it's just good practice
resulting from years of" kernel coding to have:
 - members declared strongly according to the logic of the code, and
   if no strong preference: 
 - list head be the first element of the structure, to let compiler
   avoid generating offsets when traversing lists;
 - put elements of greater size at the beginning, so no holes will be
   emitted like in the example above.

So I'd suggest:

  struct irq_sim_irq_ctx {
     struct list_head        siblings;
     struct irq_sim_work_ctx *work_ctx;
     int                     irqnum;
     bool                    enabled;
  }
  
Again, if there's NO ANY reason to have the irq number at the
beginning.

While here, I wonder, why irqnum is signed? Looking at the very first
random function in kernel/irq/irq_sim.c, I see that it's initialized
from a function returning unsigned value:

  static void irq_sim_handle_irq(struct irq_work *work)
  {
          struct irq_sim_work_ctx *work_ctx;
          unsigned int offset = 0;
          int irqnum;
  
          work_ctx = container_of(work, struct irq_sim_work_ctx, work);
  
          while (!bitmap_empty(work_ctx->pending, work_ctx->irq_count)) {
                  offset = find_next_bit(work_ctx->pending,
                                         work_ctx->irq_count, offset);
                  clear_bit(offset, work_ctx->pending);
                  irqnum = irq_find_mapping(work_ctx->domain, offset);
                  handle_simple_irq(irq_to_desc(irqnum));
          }
  }

Thanks,
Yury