linux-kernel - Re: [PATCH v1 1/4] ata: libata: Remove redundant sense

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: <ZnS5XVJaK1fWvkem@google.com>
Date: Thu, 20 Jun 2024 23:21:01 +0000
From: Igor Pylypiv <ipylypiv@...gle.com>
To: Niklas Cassel <cassel@...nel.org>
Cc: Damien Le Moal <dlemoal@...nel.org>, Tejun Heo <tj@...nel.org>,
	Hannes Reinecke <hare@...e.de>, linux-ide@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/4] ata: libata: Remove redundant sense_buffer memsets

On Thu, Jun 20, 2024 at 01:51:26PM +0200, Niklas Cassel wrote:
> On Tue, Jun 18, 2024 at 07:58:56PM +0000, Igor Pylypiv wrote:
> > 
> > I think we should explicitly memset buffers before passing them to
> > atapi_eh_request_sense() in atapi_eh_clear_ua() and zpready() so that
> > atapi_eh_request_sense() can have the same behavior as ata_eh_request_sense()
> > with regards to sense buffer expectations i.e. both functions will expect
> > buffers that are already memeset to zero.
> 
> Well, you could argue that:
> static bool ata_eh_request_sense(struct ata_queued_cmd *qc)
> doesn't take a sense_buffer, but:
> 
> unsigned int atapi_eh_request_sense(struct ata_device *dev,
>                                     u8 *sense_buf, u8 dfl_sense_key)
> 
> does, so it makes sense for atapi_eh_request_sense() to memset() the buffer.
> 
> 
> > I think that it is still benefitial to remove the redundant memset() from
> > the ata_eh_analyze_tf() -> atapi_eh_request_sense() path?
> 
> atapi_eh_request_sense() should only be called when ATA_SENSE bit is set,
> so this is only called in special circumstances, so it is not like the
> memset() is in the hot path.
> 
> If you ask me, I think that the current code is fine.
> 

I didn't think about the "takes a sense_buffer as an argument" vs "doesn't take
a sense_buffer as an argument" aspect. Yeah, keeping memset() makes sense in
this case. I'll drop the memset removal from atapi_eh_request_sense() in v2.

Thank you!
Igor

> 
> Kind regards,
> Niklas