linux-kernel - Re: [PATCH 2/3] irqchip: irq-mips-gic: Provide function to map GIC user section

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56123331.3010200@imgtec.com>
Date:	Mon, 5 Oct 2015 09:22:09 +0100
From:	Markos Chandras <Markos.Chandras@...tec.com>
To:	Marc Zyngier <marc.zyngier@....com>, <linux-mips@...ux-mips.org>
CC:	<alex@...x-smith.me.uk>, Alex Smith <alex.smith@...tec.com>,
	"Thomas Gleixner" <tglx@...utronix.de>,
	Jason Cooper <jason@...edaemon.net>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/3] irqchip: irq-mips-gic: Provide function to map GIC
 user section

Hi,

On 09/28/2015 11:55 AM, Marc Zyngier wrote:
> On 28/09/15 11:11, Markos Chandras wrote:
>> From: Alex Smith <alex.smith@...tec.com>
>>
>> The GIC provides a "user-mode visible" section containing a mirror of
>> the counter registers which can be mapped into user memory. This will
>> be used by the VDSO time function implementations, so provide a
>> function to map it in.
>>
>> When the GIC is not enabled in Kconfig a dummy inline version of this
>> function is provided, along with "#define gic_present 0", so that we
>> don't have to litter the VDSO code with ifdefs.
>>
>> Cc: Thomas Gleixner <tglx@...utronix.de>
>> Cc: Jason Cooper <jason@...edaemon.net>
>> Cc: Marc Zyngier <marc.zyngier@....com>
>> Cc: linux-kernel@...r.kernel.org
>> Signed-off-by: Alex Smith <alex.smith@...tec.com>
>> Signed-off-by: Markos Chandras <markos.chandras@...tec.com>
>> ---
>>  drivers/irqchip/irq-mips-gic.c   | 27 +++++++++++++++++++++------
>>  include/linux/irqchip/mips-gic.h | 24 ++++++++++++++++++++++--
>>  2 files changed, 43 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
>> index af2f16bb8a94..c995b199ca32 100644
>> --- a/drivers/irqchip/irq-mips-gic.c
>> +++ b/drivers/irqchip/irq-mips-gic.c
>> @@ -13,6 +13,7 @@
>>  #include <linux/irq.h>
>>  #include <linux/irqchip.h>
>>  #include <linux/irqchip/mips-gic.h>
>> +#include <linux/mm.h>
>>  #include <linux/of_address.h>
>>  #include <linux/sched.h>
>>  #include <linux/smp.h>
>> @@ -29,6 +30,7 @@ struct gic_pcpu_mask {
>>  	DECLARE_BITMAP(pcpu_mask, GIC_MAX_INTRS);
>>  };
>>  
>> +static unsigned long gic_base_addr;
>>  static void __iomem *gic_base;
>>  static struct gic_pcpu_mask pcpu_masks[NR_CPUS];
>>  static DEFINE_SPINLOCK(gic_lock);
>> @@ -301,6 +303,19 @@ int gic_get_c0_fdc_int(void)
>>  				  GIC_LOCAL_TO_HWIRQ(GIC_LOCAL_INT_FDC));
>>  }
>>  
>> +int gic_map_user_section(struct vm_area_struct *vma, unsigned long base,
>> +			 unsigned long size)
>> +{
>> +	unsigned long pfn;
>> +
>> +	BUG_ON(!gic_present);
> 
> Why do you have a BUG() here, while you're just returning -1 in the case
> where CONFIG_MIPS_GIC is not refined? This feels overly harsh to me.

I suppose i could change that to return -1 if git_present is not true.

> 
>> +	BUG_ON(size > USM_VISIBLE_SECTION_SIZE);
> 
> Same here.

But I think this is different. The size of mapping has to be less than
USM_VISIBLE_SECTION_SIZE because that's the maximum data size exposed by
the GIC chip for userspace use. So if that's not true, then BUG_ON seems
like a sensible thing to do.

> 
> - Does this code have to be in the irqchip driver? It really feels out
> of place, and I'd rather see a function that returns the mappable range
> to the VDSO code, where the mapping would occur.
> 
> Thanks,
> 

That does seem like a good idea. I will have a look


-- 
markos
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/