linux-kernel - Re: [PATCH] pata_parport: add driver (PARIDE replacement)

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: <61c81839-ac6a-32c1-97a8-f6fefb8642d7@opensource.wdc.com>
Date:   Wed, 16 Mar 2022 09:19:22 +0900
From:   Damien Le Moal <damien.lemoal@...nsource.wdc.com>
To:     Ondrej Zary <linux@...y.sk>, Jens Axboe <axboe@...nel.dk>
Cc:     Christoph Hellwig <hch@....de>, Tim Waugh <tim@...erelk.net>,
        linux-block@...r.kernel.org, linux-parport@...ts.infradead.org,
        linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] pata_parport: add driver (PARIDE replacement)

On 3/16/22 06:17, Ondrej Zary wrote:
>>> Something like this? Requires Mike's SCSI BLK_MQ_F_BLOCKING patch:
>>> https://lore.kernel.org/all/20220308003957.123312-2-michael.christie%40oracle.com/
>>>
>>> #define PATA_PARPORT_SHT(drv_name)      \
>>>         ATA_PIO_SHT(drv_name),          \
>>>         .queuecommand_blocks    = true,
>>>
>>> static void pi_connect(struct ata_port *ap)
>>> {
>>> 	struct pi_adapter *pi = ap->host->private_data;
>>>
>>> 	del_timer_sync(&pi->timer);
>>> 	if (!pi->claimed) {
>>> 		bool locked = spin_is_locked(ap->lock);

For the pi_connect() call in the ata_qc_issue() context, ap->lock is
always held, so this is not necessary.

If you have other pi_connect() calls in different contexts, we will need
to address these too. For internal commands during scan, ap->lock is
also always held.

>>> 		pi->claimed = true;
>>> 		if (locked)
>>> 			spin_unlock(ap->lock);

You need spin_unlock_irqrestore(). See the locking done in
ata_scsi_queuecmd() which is the starting point for issuing a command
through libata.

>>> 		parport_claim_or_block(pi->pardev);
>>> 		if (locked)
>>> 			spin_lock(ap->lock);
>>> 		pi->proto->connect(pi);
>>> 	}
>>> }
>>>
>>> spin_is_locked is needed because the lock is not always held. It seems
>>> to work - no more stack traces after device double registration (only
>>> ATA errors but that's expected).
>>
>> That's a very bad paradigm. What if it is locked, but the caller isn't
>> the one that locked it? Would be better to either make the locking state
>> consistent, or provide an unlocked variant (if feasible, doesn't always
>> work if it's a provided helper already in a struct of ops), or even
>> resorting to passing in locking state as a last resort.
>  
> libata locking seems to be very complex and our functions seem to be called with various lock states. I'm lost.
> 
> Might be easier to add connect() and disconnect() to struct ata_port_operations...

But you would not be able to call these within the ata_qc_issue()
context, which I think is necessary in your case. Also, these
connect/disconnect operations are not something defined by the ATA
protocol, so we should try to keep these hidden in the LLDD. It is
better I think to find a solution about the locking, if necessary using
a different qc_issue operation or using the queuecommand_blocks
attribute to have libata call the LLDD qc_issue without lock helds,
which should be OK (need to check).

Ideally, we should refine this ap->lock big lock to avoid it being held
throughout the entire submission path. I will try to have a look at this.

-- 
Damien Le Moal
Western Digital Research