Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1546147 > unrolled thread

[PATCH v3 3/3] nfc: trf7970a: Prevent repeated polling from crashing the kernel

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 3/3] nfc: trf7970a: Prevent repeated polling from crashing the kernel Geoff Lansberry <geoff@kuvee.com> - 2016-12-22 05:20 +0100
    Re: [PATCH v3 3/3] nfc: trf7970a: Prevent repeated polling from  crashing the kernel Mark Greer <mgreer@animalcreek.com> - 2016-12-24 07:10 +0100
      Re: [PATCH v3 3/3] nfc: trf7970a: Prevent repeated polling from  crashing the kernel Geoff Lansberry <geoff@kuvee.com> - 2016-12-24 17:20 +0100
        Re: [PATCH v3 3/3] nfc: trf7970a: Prevent repeated polling from  crashing the kernel Mark Greer <mgreer@animalcreek.com> - 2016-12-24 18:30 +0100
          Re: [PATCH v3 3/3] nfc: trf7970a: Prevent repeated polling from  crashing the kernel Geoff Lansberry <geoff@kuvee.com> - 2016-12-27 15:30 +0100
            Re: [PATCH v3 3/3] nfc: trf7970a: Prevent repeated polling from  crashing the kernel Mark Greer <mgreer@animalcreek.com> - 2017-01-03 17:50 +0100

#1546147 — [PATCH v3 3/3] nfc: trf7970a: Prevent repeated polling from crashing the kernel

From: Jaret Cantu <jaret.cantu@timesys.com>

Repeated polling attempts cause a NULL dereference error to occur.
This is because the state of the trf7970a is currently reading but
another request has been made to send a command before it has finished.

The solution is to properly kill the waiting reading (workqueue)
before failing on the send.

Signed-off-by: Geoff Lansberry <geoff@kuvee.com>
---
 drivers/nfc/trf7970a.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
index e3c72c6..ba5f9b8 100644
--- a/drivers/nfc/trf7970a.c
+++ b/drivers/nfc/trf7970a.c
@@ -1496,6 +1496,10 @@ static int trf7970a_send_cmd(struct nfc_digital_dev *ddev,
 			(trf->state != TRF7970A_ST_IDLE_RX_BLOCKED)) {
 		dev_err(trf->dev, "%s - Bogus state: %d\n", __func__,
 				trf->state);
+		if (trf->state == TRF7970A_ST_WAIT_FOR_RX_DATA ||
+		    trf->state == TRF7970A_ST_WAIT_FOR_RX_DATA_CONT)
+			trf->ignore_timeout =
+				!cancel_delayed_work(&trf->timeout_work);
 		ret = -EIO;
 		goto out_err;
 	}
-- 
2.7.4

[toc] | [next] | [standalone]

#1547028 — Re: [PATCH v3 3/3] nfc: trf7970a: Prevent repeated polling from crashing the kernel

On Wed, Dec 21, 2016 at 11:18:34PM -0500, Geoff Lansberry wrote:
> From: Jaret Cantu <jaret.cantu@timesys.com>
> 
> Repeated polling attempts cause a NULL dereference error to occur.
> This is because the state of the trf7970a is currently reading but
> another request has been made to send a command before it has finished.
> 
> The solution is to properly kill the waiting reading (workqueue)
> before failing on the send.
> 
> Signed-off-by: Geoff Lansberry <geoff@kuvee.com>
> ---

You've still provided virtually no information on the actual problem(s)
nor justified why you think this is the best solution.  You're adding
code to a section of code that should _never_ be executed so the only
reasonable things I can infer is that there are, at least, two problems:

1) There is a bug causing execution to get into this block of code.

2) Once in this block of code, there is another bug.

You seem to be attempting to fix 2) and completely ignoring 1).
1) is the first bug that needs to be root-caused and fixed.

Also, what exactly is the "NULL dereference error" you mention?
Is this the neard crash you talked about in another thread or is
this a kernel crash?  If it is the kernel crash, please post the
relevant information.  If this is the neard crash - which seems
unlikely - then how can changing a section of kernel code that
shouldn't be executed in the first place fix that?

Mark
--

[toc] | [prev] | [next] | [standalone]

#1547078 — Re: [PATCH v3 3/3] nfc: trf7970a: Prevent repeated polling from crashing the kernel

Mark - I'm sorry, but I did not write this code, and therefore was not
able to accurately describe it.   It is fixing a different issue, not
the neard segfault that we are still chasing. Last week Jaret Cantu
sent a separate email explaining the purpose of the code, which had
you copied, did you see that?   Does it explain why it was done to
your satisfaction?   I've asked him to join in on the effort to push
the change upstream, however he will not be available until the new
year.

I know you did suggest that we split off that change from the others,
and if now is the time to do that, let me know.   If you don't have
the email from Jaret, also please let me know and I will forward it to
you.

Geoff
Geoff Lansberry


Engineering Guy
Kuvée, Inc
125 Kingston St., 3rd Floor
Boston, MA 02111
1-617-290-1118 (m)
geoff.lansberry (skype)
http://www.kuvee.com



On Sat, Dec 24, 2016 at 1:01 AM, Mark Greer <mgreer@animalcreek.com> wrote:
> On Wed, Dec 21, 2016 at 11:18:34PM -0500, Geoff Lansberry wrote:
>> From: Jaret Cantu <jaret.cantu@timesys.com>
>>
>> Repeated polling attempts cause a NULL dereference error to occur.
>> This is because the state of the trf7970a is currently reading but
>> another request has been made to send a command before it has finished.
>>
>> The solution is to properly kill the waiting reading (workqueue)
>> before failing on the send.
>>
>> Signed-off-by: Geoff Lansberry <geoff@kuvee.com>
>> ---
>
> You've still provided virtually no information on the actual problem(s)
> nor justified why you think this is the best solution.  You're adding
> code to a section of code that should _never_ be executed so the only
> reasonable things I can infer is that there are, at least, two problems:
>
> 1) There is a bug causing execution to get into this block of code.
>
> 2) Once in this block of code, there is another bug.
>
> You seem to be attempting to fix 2) and completely ignoring 1).
> 1) is the first bug that needs to be root-caused and fixed.
>
> Also, what exactly is the "NULL dereference error" you mention?
> Is this the neard crash you talked about in another thread or is
> this a kernel crash?  If it is the kernel crash, please post the
> relevant information.  If this is the neard crash - which seems
> unlikely - then how can changing a section of kernel code that
> shouldn't be executed in the first place fix that?
>
> Mark
> --

[toc] | [prev] | [next] | [standalone]

#1547092 — Re: [PATCH v3 3/3] nfc: trf7970a: Prevent repeated polling from crashing the kernel

On Sat, Dec 24, 2016 at 11:17:18AM -0500, Geoff Lansberry wrote:
> Mark - I'm sorry, but I did not write this code, and therefore was not
> able to accurately describe it.   It is fixing a different issue, not
> the neard segfault that we are still chasing. Last week Jaret Cantu
> sent a separate email explaining the purpose of the code, which had
> you copied, did you see that?

Hm, no, I didn't.  I received an email from Justin Bronder but not from
Jaret Cantu.  Justin's email did help but is still pretty high-level.
We need a clear understanding as to what is happening in the digital
layer and the driver to know how execution is getting into a block of
error handling code that should never be executed.  Once we understand
that we can start thinking about what the best fix is.

> Does it explain why it was done to
> your satisfaction?   I've asked him to join in on the effort to push
> the change upstream, however he will not be available until the new
> year.

I expect that it would help if he joins.  After the holidays is fine -
I think many people are taking it easy for the next week or so, anyway.

> I know you did suggest that we split off that change from the others,
> and if now is the time to do that, let me know.   If you don't have
> the email from Jaret, also please let me know and I will forward it to
> you.

I think it would help you if you split it off because the first two patches
have a good chance of being accepted but this one doesn't (yet).  If you
separate the them, it will make it easier for Samuel to take the first two
(or he may take the first two anyway but its always good to make it as
easy maintainers as you can).

Mark
--

[toc] | [prev] | [next] | [standalone]

#1547611 — Re: [PATCH v3 3/3] nfc: trf7970a: Prevent repeated polling from crashing the kernel

Mark - I will split this off soon.

In the meantime - here is some more info about how we use it.

We do use NFC structures.    I did find an interesting clue in that
there are certain bottles that cause neard to segfault,  I'm not sure
what is different about them.  We write a string, like
"coppola_chardonnay_2015" to the bottles.  Come to think of it, I
haven't done anything special to make that an ndef record, just
assumed that it would happen by default, I'll look into this further.
  Also, I've been running neard with --plugin nfctype2. Just in case
the problem was happening due to cycling through other tag types.   It
didn't seem to make any difference, but I have not gone back to
default.

Geoff
Geoff Lansberry


Engineering Guy
Kuvée, Inc
125 Kingston St., 3rd Floor
Boston, MA 02111
1-617-290-1118 (m)
geoff.lansberry (skype)
http://www.kuvee.com



On Sat, Dec 24, 2016 at 12:24 PM, Mark Greer <mgreer@animalcreek.com> wrote:
> On Sat, Dec 24, 2016 at 11:17:18AM -0500, Geoff Lansberry wrote:
>> Mark - I'm sorry, but I did not write this code, and therefore was not
>> able to accurately describe it.   It is fixing a different issue, not
>> the neard segfault that we are still chasing. Last week Jaret Cantu
>> sent a separate email explaining the purpose of the code, which had
>> you copied, did you see that?
>
> Hm, no, I didn't.  I received an email from Justin Bronder but not from
> Jaret Cantu.  Justin's email did help but is still pretty high-level.
> We need a clear understanding as to what is happening in the digital
> layer and the driver to know how execution is getting into a block of
> error handling code that should never be executed.  Once we understand
> that we can start thinking about what the best fix is.
>
>> Does it explain why it was done to
>> your satisfaction?   I've asked him to join in on the effort to push
>> the change upstream, however he will not be available until the new
>> year.
>
> I expect that it would help if he joins.  After the holidays is fine -
> I think many people are taking it easy for the next week or so, anyway.
>
>> I know you did suggest that we split off that change from the others,
>> and if now is the time to do that, let me know.   If you don't have
>> the email from Jaret, also please let me know and I will forward it to
>> you.
>
> I think it would help you if you split it off because the first two patches
> have a good chance of being accepted but this one doesn't (yet).  If you
> separate the them, it will make it easier for Samuel to take the first two
> (or he may take the first two anyway but its always good to make it as
> easy maintainers as you can).
>
> Mark
> --

[toc] | [prev] | [next] | [standalone]

#1549949 — Re: [PATCH v3 3/3] nfc: trf7970a: Prevent repeated polling from crashing the kernel

[Please stop top-posting.  Bottom-post only to these lists.]

Hi Geoff & happy new year.

On Tue, Dec 27, 2016 at 09:18:32AM -0500, Geoff Lansberry wrote:
> Mark - I will split this off soon.

OK

> In the meantime - here is some more info about how we use it.
> 
> We do use NFC structures.    I did find an interesting clue in that
> there are certain bottles that cause neard to segfault,  I'm not sure
> what is different about them.  We write a string, like
> "coppola_chardonnay_2015" to the bottles.

Off the top of my head, it could be the length of the text.
It would be useful to compare the data that works to the data
that doesn't work.  Can you install NXP's 'TagInfo' app on a
smartphone and scan tags with working & non-working data?
You can email the data from the app to yourself, edit out
the cruft, and share here.

> Come to think of it, I
> haven't done anything special to make that an ndef record, just
> assumed that it would happen by default, I'll look into this further.

If you wrote the data using neard, it will be NDEF formatted.
Since it is working this well, it is virtually guaranteed that
the data is NDEF formatted.

>   Also, I've been running neard with --plugin nfctype2. Just in case
> the problem was happening due to cycling through other tag types.   It
> didn't seem to make any difference, but I have not gone back to
> default.

Good to know, thanks.

Mark
--

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web