Re: [PULL 1/1] vfio-ccw: Don't assume there are more ccws after a TIC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 02/21/2019 10:38 AM, Eric Farman wrote:


On 02/21/2019 07:55 AM, Halil Pasic wrote:
On Thu, 21 Feb 2019 10:30:01 +0100
Cornelia Huck <cohuck@xxxxxxxxxx> wrote:

On Wed, 20 Feb 2019 22:02:10 -0500
Eric Farman <farman@xxxxxxxxxxxxx> wrote:

On 02/20/2019 05:35 PM, Eric Farman wrote:


On 02/20/2019 11:44 AM, Cornelia Huck wrote:
On Wed, 20 Feb 2019 11:25:46 -0500
Eric Farman <farman@xxxxxxxxxxxxx> wrote:
On 02/20/2019 07:44 AM, Cornelia Huck wrote:
On Wed, 20 Feb 2019 06:29:38 -0500
Eric Farman <farman@xxxxxxxxxxxxx> wrote:
On 02/20/2019 04:48 AM, Cornelia Huck wrote:
On Tue, 19 Feb 2019 21:49:07 -0500
Eric Farman <farman@xxxxxxxxxxxxx> wrote:
Hi Connie, Farhan,

On 02/04/2019 12:06 PM, Cornelia Huck wrote:
From: Farhan Ali <alifm@xxxxxxxxxxxxx>

When trying to calculate the length of a ccw chain, we assume
there are ccws after a TIC. This can lead to overcounting and
copying garbage data from guest memory.

Signed-off-by: Farhan Ali <alifm@xxxxxxxxxxxxx>
Message-Id:
<d63748c1f1b03147bcbf401596638627a5e35ef7.1548082107.git.alifm@xxxxxxxxxxxxx>

Reviewed-by: Halil Pasic <pasic@xxxxxxxxxxxxx>
Signed-off-by: Cornelia Huck <cohuck@xxxxxxxxxx>
---
      drivers/s390/cio/vfio_ccw_cp.c | 2 +-
      1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c
b/drivers/s390/cio/vfio_ccw_cp.c
index 70a006ba4d05..ba08fe137c2e 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -392,7 +392,7 @@ static int ccwchain_calc_length(u64 iova,
struct channel_program *cp)
                  return -EOPNOTSUPP;
              }
-        if ((!ccw_is_chain(ccw)) && (!ccw_is_tic(ccw)))
+        if (!ccw_is_chain(ccw))

OK, this function now returns the length of the chain excluding the
last tic.

Hm, not so sure about that anymore. I'm a bit tired, please apply salt
where needed to what I'm seeing.

I didn't get as far as I had hoped today, so I'm going to need to reset
to tomorrow, have coffee, and try again.  But it does seem that with
this patch, we calculate the length of the chain up to and including the
TIC, and nothing beyond it.

That is, during the boot process we calculate a chain length of "3" for
a SEEK + SIDE + TIC that QEMU built.

Hm... so it looks like that code does what it says on the tin. But why
are we missing the second round, that should give us a chain with the
forth ccw? Are we missing a loop somewhere?


I think this is close...  More below.


Have you seen this email of mine:
https://www.spinics.net/lists/kvm/msg182535.html
(MID: Message-Id: <20190220143729.6dda5665@oc2783563651>)?

Eric told me the TIC points to the SIDE.

I thought I put that in one of my responses, but regardless it's what the bios code builds so it's not new news.

  I.e the channel program says
loop until SIDE presents a status modifier the jump over the TIC and
continue with the READ.

I needed to get past the incorrect length error before I could give Halil's email any consideration.  Now that that is cleared up, we can look into his idea.


The point is the fourth ccw is only reachable if we jump over the TIC
because of status modifier (from device). Currently we do not seem to
care about this 'jump over a ccw' possibility. IMHO that is where this
problem comes from.

Well, sort of...  We probably care about the "jump over a CCW" possibility if the TIC introduces any recursion.  If it goes "somewhere new", then the likelihood of the Status Modifier coming into play is low.

And besides, in this scenario we don't know about a potential Status Modifier until the SIDE runs, which means while we're building the channel program we don't know whether the memory after the TIC is interesting or not.  Reverting this patch makes the "recursion" scenario (which the bios introduces, and thus brings about the importance of the Status Modifier) work again, but then we have difficulties when the TIC branches somewhere else.  (See the use of NOP+TIC chains in error recovery.)

If you're suggesting adding code to handle a Status Modifier, I would ask why the interrupt handler should redrive things after the TIC, when it would be simpler to detect the TIC recursion and planning for the possibility there.  That would seem simpler than leaving a window open between (for example) orientation and data transfer.

The difficulty, as Connie pointed out, is that we're still building the current chain here, which implies we can't rely on tic_target_chain_exists() to determine whether we are dealing with a TIC loop or not.  I had some ideas this morning during the commute that might apply here, and warrant some experimentation.  Let me cobble this together and see what comes from it.

Well, taking the TIC-boundary logic from tic_target_chain_exists() and stuffing it into ccwchain_calc_length() seems to do the trick for seeing if we're sending a TIC back into our current chain, and makes the dasd boot work again (the following is built on top of this patch, and is surely whitespace damaged):

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index ba08fe137c2e..a4abe7519b7e 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -362,6 +362,7 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
 {
        struct ccw1 *ccw, *p;
        int cnt;
+       u64 tail;

        /*
         * Copy current chain from guest to host kernel.
@@ -392,7 +393,9 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
                        return -EOPNOTSUPP;
                }

-               if (!ccw_is_chain(ccw))
+               tail = iova + (cnt - 1) * sizeof(struct ccw1);
+
+ if (!(ccw_is_chain(ccw) || (ccw_is_tic(ccw) && iova <= ccw->cda && ccw->cda <= tail)))
                        break;

                ccw++;

I need to spend some more time looking over whether we should look at other chains by also calling tic_target_chain_exists(), and of course see what happens to this when we run the original I/O exercisers that led Farhan to the original patch. But it looks promising. Thoughts?

 - Eric




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux