[PATCH] pinctrl: samsung: fix segfault when using external interrupts on s3c24xx

[PATCH] pinctrl: samsung: fix segfault when using external interrupts on s3c24xx

[Sergio Prado]

We are getting a NULL pointer dereference when working with external
interrupts on s3c24xx:

Unable to handle kernel NULL pointer dereference at virtual address 000000a8
pgd = c0104000
[000000a8] *pgd=00000000
Internal error: Oops: 5 [#1] ARM
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted 4.10.0-rc8mini2440-sub-00047-g0722f57bfae9-dirty #3
Hardware name: Samsung S3C2416 (Flattened Device Tree)
task: c07399f8 task.stack: c0734000
PC is at s3c24xx_demux_eint4_7+0x24/0x10c
LR is at s3c24xx_demux_eint4_7+0x108/0x10c

The problem is in the function s3c24xx_demux_eint() when dereferencing
bank->eint_base.

At this point, we cannot get the bank pointer from the irq_desc
structure since it is pointing to the hardware irq, not virq.

So let's get the bank pointer directly from data->drvdata.

This is a regression from commit 8b1bd11c1f8f529057369c5b3702d13fd24e2765.

Tested on FriendlyARM mini2440.

Signed-off-by: Sergio Prado <sergio.prado@e-labworks.com>
---
 drivers/pinctrl/samsung/pinctrl-s3c24xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
index b82a003546ae..1b8d887796e8 100644
--- a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
+++ b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
@@ -356,8 +356,8 @@ static inline void s3c24xx_demux_eint(struct irq_desc *desc,
 {
 	struct s3c24xx_eint_data *data = irq_desc_get_handler_data(desc);
 	struct irq_chip *chip = irq_desc_get_chip(desc);
-	struct irq_data *irqd = irq_desc_get_irq_data(desc);
-	struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
+	struct samsung_pinctrl_drv_data *d = data->drvdata;
+	struct samsung_pin_bank *bank = d->pin_banks;
 	unsigned int pend, mask;
 
 	chained_irq_enter(chip, desc);
-- 
1.9.1

Re: [PATCH] pinctrl: samsung: fix segfault when using external interrupts on s3c24xx

[Krzysztof Kozlowski]

On Thu, Mar 02, 2017 at 06:23:19PM -0300, Sergio Prado wrote:
> We are getting a NULL pointer dereference when working with external
> interrupts on s3c24xx:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 000000a8
> pgd = c0104000
> [000000a8] *pgd=00000000
> Internal error: Oops: 5 [#1] ARM
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper Not tainted 4.10.0-rc8mini2440-sub-00047-g0722f57bfae9-dirty #3
> Hardware name: Samsung S3C2416 (Flattened Device Tree)
> task: c07399f8 task.stack: c0734000
> PC is at s3c24xx_demux_eint4_7+0x24/0x10c
> LR is at s3c24xx_demux_eint4_7+0x108/0x10c
> 
> The problem is in the function s3c24xx_demux_eint() when dereferencing
> bank->eint_base.
> 
> At this point, we cannot get the bank pointer from the irq_desc
> structure since it is pointing to the hardware irq, not virq.
> 
> So let's get the bank pointer directly from data->drvdata.
> 
> This is a regression from commit 8b1bd11c1f8f529057369c5b3702d13fd24e2765.

Checkpatch should complain here about commit format.

> 
> Tested on FriendlyARM mini2440.
> 

Please add:
  Fixes: 8b1bd11c1f8f ("pinctrl: samsung: Add the support the multiple IORESOURCE_MEM for one pin-bank")
  Cc: <stable@vger.kernel.org>

> Signed-off-by: Sergio Prado <sergio.prado@e-labworks.com>
> ---
>  drivers/pinctrl/samsung/pinctrl-s3c24xx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
> index b82a003546ae..1b8d887796e8 100644
> --- a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
> +++ b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
> @@ -356,8 +356,8 @@ static inline void s3c24xx_demux_eint(struct irq_desc *desc,
>  {
>  	struct s3c24xx_eint_data *data = irq_desc_get_handler_data(desc);
>  	struct irq_chip *chip = irq_desc_get_chip(desc);
> -	struct irq_data *irqd = irq_desc_get_irq_data(desc);
> -	struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
> +	struct samsung_pinctrl_drv_data *d = data->drvdata;
> +	struct samsung_pin_bank *bank = d->pin_banks;

I think 'pin_banks' point to all banks of given controller not to the
currently accessed one.


Best regards,
Krzysztof

Re: [PATCH] pinctrl: samsung: fix segfault when using external interrupts on s3c24xx

[Tomasz Figa]

[+Chanwoo]

2017-03-04 17:15 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>:
> On Thu, Mar 02, 2017 at 06:23:19PM -0300, Sergio Prado wrote:
>> We are getting a NULL pointer dereference when working with external
>> interrupts on s3c24xx:
>>
>> Unable to handle kernel NULL pointer dereference at virtual address 000000a8
>> pgd = c0104000
>> [000000a8] *pgd=00000000
>> Internal error: Oops: 5 [#1] ARM
>> Modules linked in:
>> CPU: 0 PID: 0 Comm: swapper Not tainted 4.10.0-rc8mini2440-sub-00047-g0722f57bfae9-dirty #3
>> Hardware name: Samsung S3C2416 (Flattened Device Tree)
>> task: c07399f8 task.stack: c0734000
>> PC is at s3c24xx_demux_eint4_7+0x24/0x10c
>> LR is at s3c24xx_demux_eint4_7+0x108/0x10c
>>
>> The problem is in the function s3c24xx_demux_eint() when dereferencing
>> bank->eint_base.
>>
>> At this point, we cannot get the bank pointer from the irq_desc
>> structure since it is pointing to the hardware irq, not virq.
>>
>> So let's get the bank pointer directly from data->drvdata.
>>
>> This is a regression from commit 8b1bd11c1f8f529057369c5b3702d13fd24e2765.
>
> Checkpatch should complain here about commit format.
>
>>
>> Tested on FriendlyARM mini2440.
>>
>
> Please add:
>   Fixes: 8b1bd11c1f8f ("pinctrl: samsung: Add the support the multiple IORESOURCE_MEM for one pin-bank")
>   Cc: <stable@vger.kernel.org>

Yeah, that patch seems to have also affected s3c64xx...

>
>> Signed-off-by: Sergio Prado <sergio.prado@e-labworks.com>
>> ---
>>  drivers/pinctrl/samsung/pinctrl-s3c24xx.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
>> index b82a003546ae..1b8d887796e8 100644
>> --- a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
>> +++ b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
>> @@ -356,8 +356,8 @@ static inline void s3c24xx_demux_eint(struct irq_desc *desc,
>>  {
>>       struct s3c24xx_eint_data *data = irq_desc_get_handler_data(desc);
>>       struct irq_chip *chip = irq_desc_get_chip(desc);
>> -     struct irq_data *irqd = irq_desc_get_irq_data(desc);
>> -     struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
>> +     struct samsung_pinctrl_drv_data *d = data->drvdata;
>> +     struct samsung_pin_bank *bank = d->pin_banks;
>
> I think 'pin_banks' point to all banks of given controller not to the
> currently accessed one.

Yeah, this doesn't seem to be a logically correct fix, although it
should work since in s3c24xx all banks would have the same eint_base
value.

I would suggest a partial revert of 8b1bd11c1f8f, i.e. bringing back
samsung_pinctrl_drv_data::virt_base and undoing the changes made in
s3c24xx and s3c64xx EINT code.

Best regards,
Tomasz

Re: [PATCH] pinctrl: samsung: fix segfault when using external interrupts on s3c24xx

[Sergio Prado]

Hi Krzysztof,

> > This is a regression from commit 8b1bd11c1f8f529057369c5b3702d13fd24e2765.
> 
> Checkpatch should complain here about commit format.
> 
> > 
> > Tested on FriendlyARM mini2440.
> > 
> 
> Please add:
>   Fixes: 8b1bd11c1f8f ("pinctrl: samsung: Add the support the multiple IORESOURCE_MEM for one pin-bank")
>   Cc: <stable@vger.kernel.org>
> 

OK.

> > Signed-off-by: Sergio Prado <sergio.prado@e-labworks.com>
> > ---
> >  drivers/pinctrl/samsung/pinctrl-s3c24xx.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
> > index b82a003546ae..1b8d887796e8 100644
> > --- a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
> > +++ b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
> > @@ -356,8 +356,8 @@ static inline void s3c24xx_demux_eint(struct irq_desc *desc,
> >  {
> >  	struct s3c24xx_eint_data *data = irq_desc_get_handler_data(desc);
> >  	struct irq_chip *chip = irq_desc_get_chip(desc);
> > -	struct irq_data *irqd = irq_desc_get_irq_data(desc);
> > -	struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
> > +	struct samsung_pinctrl_drv_data *d = data->drvdata;
> > +	struct samsung_pin_bank *bank = d->pin_banks;
> 
> I think 'pin_banks' point to all banks of given controller not to the
> currently accessed one.

Understood. I think it worked in my tests because on s3c2440 all banks
have the same eint base address.

So what do you think is the best approach to solve this problem?

> 
> 
> Best regards,
> Krzysztof
> 

-- 
Sergio Prado
Embedded Labworks
Office: +55 11 2628-3461
Mobile: +55 11 97123-3420

Re: [PATCH] pinctrl: samsung: fix segfault when using external interrupts on s3c24xx

[Krzysztof Kozlowski]

On Mon, Mar 06, 2017 at 09:15:16AM -0400, Sergio Prado wrote:
> Hi Krzysztof,
> 
> > > This is a regression from commit 8b1bd11c1f8f529057369c5b3702d13fd24e2765.
> > 
> > Checkpatch should complain here about commit format.
> > 
> > > 
> > > Tested on FriendlyARM mini2440.
> > > 
> > 
> > Please add:
> >   Fixes: 8b1bd11c1f8f ("pinctrl: samsung: Add the support the multiple IORESOURCE_MEM for one pin-bank")
> >   Cc: <stable@vger.kernel.org>
> > 
> 
> OK.
> 
> > > Signed-off-by: Sergio Prado <sergio.prado@e-labworks.com>
> > > ---
> > >  drivers/pinctrl/samsung/pinctrl-s3c24xx.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
> > > index b82a003546ae..1b8d887796e8 100644
> > > --- a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
> > > +++ b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
> > > @@ -356,8 +356,8 @@ static inline void s3c24xx_demux_eint(struct irq_desc *desc,
> > >  {
> > >  	struct s3c24xx_eint_data *data = irq_desc_get_handler_data(desc);
> > >  	struct irq_chip *chip = irq_desc_get_chip(desc);
> > > -	struct irq_data *irqd = irq_desc_get_irq_data(desc);
> > > -	struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
> > > +	struct samsung_pinctrl_drv_data *d = data->drvdata;
> > > +	struct samsung_pin_bank *bank = d->pin_banks;
> > 
> > I think 'pin_banks' point to all banks of given controller not to the
> > currently accessed one.
> 
> Understood. I think it worked in my tests because on s3c2440 all banks
> have the same eint base address.
> 
> So what do you think is the best approach to solve this problem?

Maybe you can get to this through:
	s3c24xx_eint_domain_data = s3c24xx_eint_data->domains[virq].host_data;
	s3c24xx_eint_domain_data->bank

It is getting slightly more complicated...

Best regards,
Krzysztof

Re: [PATCH] pinctrl: samsung: fix segfault when using external interrupts on s3c24xx

[Tomasz Figa]

2017-03-09 1:34 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>:
> On Mon, Mar 06, 2017 at 09:15:16AM -0400, Sergio Prado wrote:
>> Hi Krzysztof,
>>
>> > > This is a regression from commit 8b1bd11c1f8f529057369c5b3702d13fd24e2765.
>> >
>> > Checkpatch should complain here about commit format.
>> >
>> > >
>> > > Tested on FriendlyARM mini2440.
>> > >
>> >
>> > Please add:
>> >   Fixes: 8b1bd11c1f8f ("pinctrl: samsung: Add the support the multiple IORESOURCE_MEM for one pin-bank")
>> >   Cc: <stable@vger.kernel.org>
>> >
>>
>> OK.
>>
>> > > Signed-off-by: Sergio Prado <sergio.prado@e-labworks.com>
>> > > ---
>> > >  drivers/pinctrl/samsung/pinctrl-s3c24xx.c | 4 ++--
>> > >  1 file changed, 2 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
>> > > index b82a003546ae..1b8d887796e8 100644
>> > > --- a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
>> > > +++ b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
>> > > @@ -356,8 +356,8 @@ static inline void s3c24xx_demux_eint(struct irq_desc *desc,
>> > >  {
>> > >   struct s3c24xx_eint_data *data = irq_desc_get_handler_data(desc);
>> > >   struct irq_chip *chip = irq_desc_get_chip(desc);
>> > > - struct irq_data *irqd = irq_desc_get_irq_data(desc);
>> > > - struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
>> > > + struct samsung_pinctrl_drv_data *d = data->drvdata;
>> > > + struct samsung_pin_bank *bank = d->pin_banks;
>> >
>> > I think 'pin_banks' point to all banks of given controller not to the
>> > currently accessed one.
>>
>> Understood. I think it worked in my tests because on s3c2440 all banks
>> have the same eint base address.
>>
>> So what do you think is the best approach to solve this problem?
>
> Maybe you can get to this through:
>         s3c24xx_eint_domain_data = s3c24xx_eint_data->domains[virq].host_data;
>         s3c24xx_eint_domain_data->bank
>
> It is getting slightly more complicated...

How about the suggestions I made in my reply from March 4 (JST)?

Best regards,
Tomasz

Re: [PATCH] pinctrl: samsung: fix segfault when using external interrupts on s3c24xx

[Krzysztof Kozlowski]

On Thu, Mar 9, 2017 at 7:56 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> 2017-03-09 1:34 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>:
>> On Mon, Mar 06, 2017 at 09:15:16AM -0400, Sergio Prado wrote:
>>> Hi Krzysztof,
>>>
>>> > > This is a regression from commit 8b1bd11c1f8f529057369c5b3702d13fd24e2765.
>>> >
>>> > Checkpatch should complain here about commit format.
>>> >
>>> > >
>>> > > Tested on FriendlyARM mini2440.
>>> > >
>>> >
>>> > Please add:
>>> >   Fixes: 8b1bd11c1f8f ("pinctrl: samsung: Add the support the multiple IORESOURCE_MEM for one pin-bank")
>>> >   Cc: <stable@vger.kernel.org>
>>> >
>>>
>>> OK.
>>>
>>> > > Signed-off-by: Sergio Prado <sergio.prado@e-labworks.com>
>>> > > ---
>>> > >  drivers/pinctrl/samsung/pinctrl-s3c24xx.c | 4 ++--
>>> > >  1 file changed, 2 insertions(+), 2 deletions(-)
>>> > >
>>> > > diff --git a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
>>> > > index b82a003546ae..1b8d887796e8 100644
>>> > > --- a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
>>> > > +++ b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
>>> > > @@ -356,8 +356,8 @@ static inline void s3c24xx_demux_eint(struct irq_desc *desc,
>>> > >  {
>>> > >   struct s3c24xx_eint_data *data = irq_desc_get_handler_data(desc);
>>> > >   struct irq_chip *chip = irq_desc_get_chip(desc);
>>> > > - struct irq_data *irqd = irq_desc_get_irq_data(desc);
>>> > > - struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
>>> > > + struct samsung_pinctrl_drv_data *d = data->drvdata;
>>> > > + struct samsung_pin_bank *bank = d->pin_banks;
>>> >
>>> > I think 'pin_banks' point to all banks of given controller not to the
>>> > currently accessed one.
>>>
>>> Understood. I think it worked in my tests because on s3c2440 all banks
>>> have the same eint base address.
>>>
>>> So what do you think is the best approach to solve this problem?
>>
>> Maybe you can get to this through:
>>         s3c24xx_eint_domain_data = s3c24xx_eint_data->domains[virq].host_data;
>>         s3c24xx_eint_domain_data->bank
>>
>> It is getting slightly more complicated...
>
> How about the suggestions I made in my reply from March 4 (JST)?

Yes, this also looks like solution. I am not sure how much you would
like to revert but wouldn't it create duplicated members in pinctrl
structures? One for Exynos and other for S3C?

Best regards,
Krzysztof

Re: [PATCH] pinctrl: samsung: fix segfault when using external interrupts on s3c24xx

[Tomasz Figa]

2017-03-09 15:42 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>:
> On Thu, Mar 9, 2017 at 7:56 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> 2017-03-09 1:34 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>:
>>> On Mon, Mar 06, 2017 at 09:15:16AM -0400, Sergio Prado wrote:
>>>> Hi Krzysztof,
>>>>
>>>> > > This is a regression from commit 8b1bd11c1f8f529057369c5b3702d13fd24e2765.
>>>> >
>>>> > Checkpatch should complain here about commit format.
>>>> >
>>>> > >
>>>> > > Tested on FriendlyARM mini2440.
>>>> > >
>>>> >
>>>> > Please add:
>>>> >   Fixes: 8b1bd11c1f8f ("pinctrl: samsung: Add the support the multiple IORESOURCE_MEM for one pin-bank")
>>>> >   Cc: <stable@vger.kernel.org>
>>>> >
>>>>
>>>> OK.
>>>>
>>>> > > Signed-off-by: Sergio Prado <sergio.prado@e-labworks.com>
>>>> > > ---
>>>> > >  drivers/pinctrl/samsung/pinctrl-s3c24xx.c | 4 ++--
>>>> > >  1 file changed, 2 insertions(+), 2 deletions(-)
>>>> > >
>>>> > > diff --git a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
>>>> > > index b82a003546ae..1b8d887796e8 100644
>>>> > > --- a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
>>>> > > +++ b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
>>>> > > @@ -356,8 +356,8 @@ static inline void s3c24xx_demux_eint(struct irq_desc *desc,
>>>> > >  {
>>>> > >   struct s3c24xx_eint_data *data = irq_desc_get_handler_data(desc);
>>>> > >   struct irq_chip *chip = irq_desc_get_chip(desc);
>>>> > > - struct irq_data *irqd = irq_desc_get_irq_data(desc);
>>>> > > - struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
>>>> > > + struct samsung_pinctrl_drv_data *d = data->drvdata;
>>>> > > + struct samsung_pin_bank *bank = d->pin_banks;
>>>> >
>>>> > I think 'pin_banks' point to all banks of given controller not to the
>>>> > currently accessed one.
>>>>
>>>> Understood. I think it worked in my tests because on s3c2440 all banks
>>>> have the same eint base address.
>>>>
>>>> So what do you think is the best approach to solve this problem?
>>>
>>> Maybe you can get to this through:
>>>         s3c24xx_eint_domain_data = s3c24xx_eint_data->domains[virq].host_data;
>>>         s3c24xx_eint_domain_data->bank
>>>
>>> It is getting slightly more complicated...
>>
>> How about the suggestions I made in my reply from March 4 (JST)?
>
> Yes, this also looks like solution. I am not sure how much you would
> like to revert but wouldn't it create duplicated members in pinctrl
> structures? One for Exynos and other for S3C?

I would actually lean towards introducing one duplicated member
(drvdata->pctl_base) versus adding unnecessarily complex special ways
of calculating the addresses from current (unsuitable) set of data.


By the way, I think I just found yet another bug introduced by that
patch. In exynos_irq_request_resources() and
exynos_irq_release_resources(), the address of CON register is
calculated from bank->eint_base, while I believe it should be
calculated from bank->pctl_base (which is also what
samsung_pinmux_setup() uses).

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/pinctrl/samsung/pinctrl-exynos.c?id=refs/tags/next-20170320#n174

Ehh, I think I regret letting that patch be merged as it made some of
the code quite messy and we actually found a better way to support
those strange banks later (group banks by their pctl_base and only
allow eint_base to be selectable per bank; then we could just keep
drvdata->pctl_base constant over all related banks).

Best regards,
Tomasz