linux-kernel - Re: [PATCH v6 0/6] g_NCR5380: PDMA fixes and cleanup

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: <201707021651.37016.linux@rainbow-software.org>
Date:   Sun, 2 Jul 2017 16:51:36 +0200
From:   Ondrej Zary <linux@...nbow-software.org>
To:     Finn Thain <fthain@...egraphics.com.au>
Cc:     "James E.J. Bottomley" <jejb@...ux.vnet.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
        Michael Schmitz <schmitzmic@...il.com>
Subject: Re: [PATCH v6 0/6] g_NCR5380: PDMA fixes and cleanup

On Sunday 02 July 2017 05:11:27 Finn Thain wrote:
> On Sat, 1 Jul 2017, Ondrej Zary wrote:
> > The write corruption is still present - "start" must be rolled back in
> > both IRQ and timeout cases.
>
> Your original algorithm aborts the transfer for a timeout. Same with mine.

I do "start -= 2 * 128" even after timeout.

> The bug must be a elsewhere.
>
> > And 128 B is not enough , 256 is OK (why did it work last time?).
>
> When I get contradictory results it usually means I booted the wrong build
> or built the wrong branch.

I've just retested PATCHv5, it really misses 128 bytes and works if I
add "residual += 128;".

> Actually, I think that adding 128 to the residual is correct in some
> sitations, and 256 is correct in other situations.
>
> > We just wrote a buffer to the chip but the chip is writing the previous
> > one to the drive - so if a problem arises, both buffers are lost.
>
> I see. I guess we have to take buffer swaps into account.
>
> > This fixes the corruption (although the "start > 0" check seems wrong
> > now): --- a/drivers/scsi/g_NCR5380.c
> > +++ b/drivers/scsi/g_NCR5380.c
> > @@ -598,23 +598,17 @@ static inline int generic_NCR5380_psend(struct
> > NCR5380_hostdata *hostdata, CSR_HOST_BUF_NOT_RDY, 0,
> >  		                           hostdata->c400_ctl_status,
> >  		                           CSR_GATED_53C80_IRQ,
> > -		                           CSR_GATED_53C80_IRQ, HZ / 64) < 0)
> > -                       break;
> > -
> > -		if (NCR5380_read(hostdata->c400_ctl_status) &
> > -		    CSR_HOST_BUF_NOT_RDY) {
> > +		                           CSR_GATED_53C80_IRQ, HZ / 64) < 0 ||
> > +		    (NCR5380_read(hostdata->c400_ctl_status) &
> > +		     (CSR_HOST_BUF_NOT_RDY | CSR_GATED_53C80_IRQ))) {
>
> You could add a printk to the timeout branch. If it executes, something is
> seriously wrong. E.g.
>
> -	break;
> +	{ pr_err("send timeout %02x, %d/%d\n",
> NCR5380_read(hostdata->c400_ctl_status), start, len); break; }

Yes, timeouts do happen:
[ 9671.909223] send timeout 14, 3840/4096
[ 9672.978079] send timeout 14, 2816/4096
[ 9675.323751] send timeout 14, 1280/4096

> >  			/* The chip has done a 128 B buffer swap but the first
> >  			 * buffer still has not reached the SCSI bus.
> >  			 */
> >  			if (start > 0)
> > -				start -= 128;
> > +				start -= 256;
> >  			break;
> >  		}
>
> BTW, that change carries the risk of 'start' going negative and the
> residual exceeding the length of the original transfer.
>
> But I agree with you that there's a problem with the residual.
>
> If I understand correctly, the 53c400 can't do a buffer swap until the
> disk acknowledges each of the 128 bytes from the buffer. But I guess the
> first buffer is special because the disk will not see the first byte of
> the transfer until after the first buffer swap.
>
> And it appears that the last buffer is also special: we have to wait for
> CSR_HOST_BUF_NOT_RDY even after start == len otherwise we may not detect a
> failure and fix the residual. So I think the datasheet is right; we have
> to iterate until the block counter goes to zero.
>
> I think it is safe to say that when CSR_HOST_BUF_NOT_RDY, 'start' is
> between 128 and 256 B ahead of the disk. Otherwise, the host buffer is
> empty and 'start' is no more than 128 B ahead of the disk.
>
> > -		if (NCR5380_read(hostdata->c400_ctl_status) &
> > -		    CSR_GATED_53C80_IRQ)
> > -			break;
> > -
> >  		if (hostdata->io_port && hostdata->io_width == 2)
> >  			outsw(hostdata->io_port + hostdata->c400_host_buf,
> >  			      src + start, 64);
> >
> >
> > DTC seems to work too.
>
> OK. Thanks for testing. Please try the patch below on top of v6.

It misses 256B blocks. It's caused by the timeouts, this patch fixes it:

--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -598,11 +598,9 @@ static inline int generic_NCR5380_psend(struct NCR5380_hostdata *hostdata,
 		                           CSR_HOST_BUF_NOT_RDY, 0,
 		                           hostdata->c400_ctl_status,
 		                           CSR_GATED_53C80_IRQ,
-		                           CSR_GATED_53C80_IRQ, HZ / 64) < 0)
-			break;
-
-		if (NCR5380_read(hostdata->c400_ctl_status) &
-		    CSR_HOST_BUF_NOT_RDY) {
+		                           CSR_GATED_53C80_IRQ, HZ / 64) < 0 ||
+		    (NCR5380_read(hostdata->c400_ctl_status) &
+		     CSR_HOST_BUF_NOT_RDY)) {
 			/* Both 128 B buffers are in use */
 			if (start >= 128)
 				start -= 128;


-- 
Ondrej Zary