Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
To: Tudor.Ambarus@microchip.com, linux-mtd@lists.infradead.org
Cc: vigneshr@ti.com, richard@nod.at, linux-kernel@vger.kernel.org,
	stable@vger.kernel.org, matija.glavinic-pecotic.ext@nokia.com,
	miquel.raynal@bootlin.com
Subject: Re: [PATCH] mtd: spi-nor: Don't copy self-pointing struct around
Date: Wed, 7 Oct 2020 11:11:17 +0200
Message-ID: <ed864816-d363-71e0-c0c9-649486727e29@nokia.com> (raw)
In-Reply-To: <f815c4e5-eabe-af3d-efb6-87000f5ba411@microchip.com>

Hello Tudor,

On 07/10/2020 10:48, Tudor.Ambarus@microchip.com wrote:
>> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
>>
>> spi_nor_parse_sfdp() modifies the passed structure so that it points to
>> itself (params.erase_map.regions to params.erase_map.uniform_region). This
>> makes it impossible to copy the local struct anywhere else.
>>
>> Therefore only use memcpy() in backup-restore scenario. The bug may show up
>> like below:
>>
>> BUG: unable to handle page fault for address: ffffc90000b377f8
>> Oops: 0000 [#1] PREEMPT SMP NOPTI
>> CPU: 4 PID: 3500 Comm: flashcp Tainted: G           O      5.4.53-... #1
>> ...
>> RIP: 0010:spi_nor_erase+0x8e/0x5c0
>> Code: 64 24 18 89 db 4d 8b b5 d0 04 00 00 4c 89 64 24 18 4c 89 64 24 20 eb 12 a8 10 0f 85 59 02 00 00 49 83 c6 10 0f 84 4f 02 00 00 <49> 8b 06 48 89 c2 48 83 e2 c0 48 89 d1 49 03 4e 08 48 39 cb 73 d8
>> RSP: 0018:ffffc9000217fc48 EFLAGS: 00010206
>> RAX: 0000000000740000 RBX: 0000000000000000 RCX: 0000000000740000
>> RDX: ffff8884550c9980 RSI: ffff88844f9c0bc0 RDI: ffff88844ede7bb8
>> RBP: 0000000000740000 R08: ffffffff815bfbe0 R09: ffff88844f9c0bc0
>> R10: 0000000000000000 R11: 0000000000000000 R12: ffffc9000217fc60
>> R13: ffff88844ede7818 R14: ffffc90000b377f8 R15: 0000000000000000
>> FS:  00007f4699780500(0000) GS:ffff88846ff00000(0000) knlGS:0000000000000000
>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: ffffc90000b377f8 CR3: 00000004538ee000 CR4: 0000000000340fe0
>> Call Trace:
>>  part_erase+0x27/0x50
>>  mtdchar_ioctl+0x831/0xba0
>>  ? filemap_map_pages+0x186/0x3d0
>>  ? do_filp_open+0xad/0x110
>>  ? _copy_to_user+0x22/0x30
>>  ? cp_new_stat+0x150/0x180
>>  mtdchar_unlocked_ioctl+0x2a/0x40
>>  do_vfs_ioctl+0xa0/0x630
>>  ? __do_sys_newfstat+0x3c/0x60
>>  ksys_ioctl+0x70/0x80
>>  __x64_sys_ioctl+0x16/0x20
>>  do_syscall_64+0x6a/0x200
>>  ? prepare_exit_to_usermode+0x50/0xd0
>>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> RIP: 0033:0x7f46996b6817
>>
>> Fixes: 1c1d8d98e1c7 ("mtd: spi-nor: Split spi_nor_init_params()")
> I think the correct Fixes tag is:
> Fixes: c46872170a54 ("mtd: spi-nor: Move erase_map to 'struct spi_nor_flash_parameter'")

yes, I think you are right regarding the exact hash!
Thank you for checking this!

>> Cc: stable@vger.kernel.org
>> Tested-by: Baurzhan Ismagulov <ibr@radix50.net>
>> Co-developed-by: Matija Glavinic Pecotic <matija.glavinic-pecotic.ext@nokia.com>
>> Signed-off-by: Matija Glavinic Pecotic <matija.glavinic-pecotic.ext@nokia.com>
>> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
>> ---
>>  drivers/mtd/spi-nor/core.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 2add4a0..cce0670 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2701,11 +2701,10 @@ static void spi_nor_sfdp_init_params(struct spi_nor *nor)
>>
>>         memcpy(&sfdp_params, nor->params, sizeof(sfdp_params));
>>
>> -       if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
>> +       if (spi_nor_parse_sfdp(nor, nor->params)) {
>> +               memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
>>                 nor->addr_width = 0;
>>                 nor->flags &= ~SNOR_F_4B_OPCODES;
>> -       } else {
>> -               memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
> neat!
> With the Fixes tag fixed, one can add:
> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

-- 
Best regards,
Alexander Sverdlin.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

  reply index

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-05  8:48 Alexander A Sverdlin
2020-10-07  8:48 ` Tudor.Ambarus
2020-10-07  9:11   ` Alexander Sverdlin [this message]
2020-10-29  4:45 ` Vignesh Raghavendra

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ed864816-d363-71e0-c0c9-649486727e29@nokia.com \
    --to=alexander.sverdlin@nokia.com \
    --cc=Tudor.Ambarus@microchip.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=matija.glavinic-pecotic.ext@nokia.com \
    --cc=miquel.raynal@bootlin.com \
    --cc=richard@nod.at \
    --cc=stable@vger.kernel.org \
    --cc=vigneshr@ti.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git