Re: Re: Closing enbd devices

"Also sprach Dag Sverre Seljebotn:"
> > I don't use "reset" so whatever you do there is OK with me :-). Be
> > aware, however, that I somewhat doubt that reset can work as expected in
> > many useful situations, because of all the race conditions associated
> > with stopping a device.
> 
> Isn't this contrary to reset's comment:
>  * This has no dangerous race condition, as we disable the device
>  * rather than destroy it!
> ?

It is contrary to what my instinct says, but I believe it.  Presumably I
have considered the situation before now and come to the conclusion that
"reset" is a weak enough set of functions that it cannot physically harm
the kernel, but with respect to data loss or misdirection and the like,
I am not sure right now ...  what happens if the kernel waits 60s before
actually sending us a request that was conceived of by its author as
being for one device, and by the time we get it we have closed and
reopened the device for a different remote target?


> "all the race conditions" is kind of greek to me. Well, I do know what a
> race condition is, but I'm not at all familiar with all the stuff going
> on that could lead to it in this scenario.

I mean in that comment that it doesn't destroy the struct that
represents the device in the driver, and replace its listed address with
"NULL". Literally, the difference between a "closed" and an "open"
device is that closed means the struct has not been created yet.

Since the struct contains all sorts of state variables and method
pointers, it is possible that a transaction (request, whatever) that
takes place carries with it some pointers into the struct, and that
if it persists longer than the struct does, then those pointers will
be stale and cause Death and Destruction when accessed.

So I am not keen to destroy the struct, which would literally be
returning it to the "closed" state. Nor can I replace the struct with a
new one, and hang on to the old, since that would be a memory leak.

In favour of "reset", I see that it only allows itself to be used when
the device is not held open by anyone. 

Unfortunately, it doesn't know if there are requests "in flight" for
the device.

Yes, one can really see why I was not keen to allow the device to
change identity! And why I tried to make it very hard, with a signature
that can only be set once.

> Here's what I do when I need reset: Unmount the device lazily, kill
> enbd-client, call reset on the device, then some waiting (possibly
> minutes or hours), then reconnect and mount something else. No access on
> the device but the mounting is done.
> 
> - Reset checks that refcount is 0, so no references will be left from
> userspace (enbd-client).

It checks that there are no open references, yes, but it does not check
a "pending request" count.  Largely because it's impossible - well, it
tries ..  it attempts to flush kernel requests for the device after
first disabling it, I think, which should kill them and bring the number
to zero.  Also, the kernel on the last close of a device ANYWAY attempts
to flush pending writes to it and then kills any that haven't made it to
the device in time.

But these are all "attempts". There is no certainty. Many conditions
can intervene.

> - And if it is just marked disabled rather than destroyed, the kernel
> will still get any requests satisfied in the same way that it did before

If it's disabled and requests arrive, I believe they are killed as
they arrive in the request function ... yes:

         if (!(flags & ENBD_ENABLED)) {
                     ENBD_DEBUG (1, "device nd%s not enabled\n", lo->devnam);
                     ENBD_FAIL ("device not enabled.\n");
         } 

but that doesn't stop them arriving AFTER the device has been enabled
again, even if it has changed. And the kernel's attempt to flush
requests only kills pending requests .. you can race with the kernel to
submit more even as it is flushing the ones that existed.

And then you can get requests which are still in flight to/from the
server!  Well, I think those will be die because they are images of a
request that has been received in kernel, so that's OK.

But I am just not cmpletely sure.

> the reset? (since no enbd-clients are connected, that means I/O-error?)

Yes.

> - Upon reconnection with a different signature...well, one better make
> sure no references to the device is around first. Is this where you
> think the problem is?

I am simply aware that there is no absolute guarrantee that there is
not a vulnerability somewhere. It is far simpler to insist on always
having the same remote device connected, via the "signature".

But I guess an administrative override is fine. If the administrator
wants to shoot himself in the foot,that is his privilege!

> Is it enough to make sure it is not actively used (ie mounted or
> otherwise made accessible for a user), or would the kernel have
> references to it lying around for a long time afterwards? If I take care

Possibly. There is simply no audit that I can (or want) to do that can
show it.

> to stop using the device before resetting it then I'm fine? Obviously a
> reset function shouldn't be used if the device could in any way still be
> needed somewhere, but if the kernel could still give it requests after
> all access from userspace and all mounts are stopped then it is worse.

There would still be minute SMP races, where flags are being set in one
kernel thread and other things are happening in another. It looks as
though I attempted tomake atomic all such metadata changes, but it is
likely that i forgot about something somewhere.

> > Perhaps I should introduce the state "stopped"? Maybe I could add
> > the signature of the device to outgoing requests. Then if requests come
> > back in bearing a signature that does not match, I could error them.
> > 
> > That would kill quite a lot of races, no?
> 
> Hmm. But I have no chance of doing this without learning a lot more
> about enbd, and you probably wouldn't have any interest in it unless it
> would also kill a lot of other races that you are struggling with...

Well, there is some sort of kernel race on removal of the driver under
2.6 that causes D&D. I don't know what it is (Don't Do That Then). If
you are going to be playing with closing and opening devices under 2.6,
you might well find it.

And currently under UML "emulation" of 2.4 in 2.6, the driver dies when
/proc/nbdinfo  is written from userspace. I don't know why. It's
alright in "real life". Maybe I am using write_proc subtly wrong -
maybe the buffer that the routine gets is in user space, instead of
kernel space, and it only shows up on that architecture .. owww.

(list of other known mysteries removed).

You encouraged me to clean up the write_proc function anyway.  I had
also written an ascii string parser so that oen can set the signature
directly via proc. But I hadn't been able to test it becase of the
UML problem. I'll look around a bit, but I HAVE incorporated your
change - as I said. it doesn't affect me! You carefully chose a
function with only one access mode, I think.

Thanks!


Peter