[OpenAFS-devel] making lock upgrades safer

[Nathan Neulinger]

Currently, it looks like all of the protection of flock's is really
enforced on the client - there is nothing in server to prevent a badly
behaving client from unlocking someone else's lock.

It would take significant protocol changes to fix that.

Currently, lock upgrades from a client are implemented as:

	Initial Lock:
		Lock File Read

	Upgrade:
		Unlock File
		Lock File Write

here's a thought on a way to possibly make lock upgrades safer.

The code on the server is currently:


static afs_int32
HandleLocking(Vnode *targetptr, afs_int32 rights, ViceLockType
LockingType)
{
    int Time;       /* Used for time */
    int writeVnode = targetptr->changed_oldTime; /* save original status
*/

    /* Does the caller has Lock priviledges; root extends locks, however
*/
    if (LockingType != LockExtend && !(rights & PRSFS_LOCK))
    return(EACCES);
    targetptr->changed_oldTime = 1; /* locking doesn't affect any time
stamp */
    Time = FT_ApproxTime();
    switch (LockingType) {
    case LockRead:
    case LockWrite:
        if (Time > targetptr->disk.lock.lockTime)
        targetptr->disk.lock.lockTime = targetptr->disk.lock.lockCount =
0;
        Time += AFS_LOCKWAIT;
        if (LockingType == LockRead) {
        if (targetptr->disk.lock.lockCount >= 0) {
            ++(targetptr->disk.lock.lockCount);
            targetptr->disk.lock.lockTime = Time;
        } else return(EAGAIN);
        } else {
        if (targetptr->disk.lock.lockCount == 0) {
            targetptr->disk.lock.lockCount = -1;
            targetptr->disk.lock.lockTime = Time;
        } else return(EAGAIN);
        }
        break;
    case LockExtend:
        Time += AFS_LOCKWAIT;
        if (targetptr->disk.lock.lockCount != 0)
        targetptr->disk.lock.lockTime = Time;
        else return(EINVAL);
        break;
    case LockRelease:
        if ((--targetptr->disk.lock.lockCount) <= 0)
        targetptr->disk.lock.lockCount = targetptr->disk.lock.lockTime =
0;
        break;
    default:
        targetptr->changed_oldTime = writeVnode; /* restore old status
*/
        ViceLog(0, ("Illegal Locking type %d\n", LockingType));
    }
    return(0);
} /*HandleLocking*/

Since we already know that we need to rely on the client to behave
reasonably - what if we say "A client is allowed to upgrade to a Write
lock if either no lock is on the file, or if the client itself knows it
has a read lock on the file, and the server knows there is only one read
lock on the file."

Unfortunately, there doesn't appear to be any way for the client to
distinguish on an old server whether it was an invalid lock type, or it
worked, since both return 0.  So, some other code would need returned
for success for a lock upgrade. Add a new lock type - LockChange. Return
0, 1, or EAGAIN. 0 would be response from old servers, 1 would be
response from new servers if upgrade was granted, EAGAIN if can't be
granted right now. 

Alternatively, we could add a new RX call, but then we'd have to change
OldSetLock to OldOldSetLock, and would have to coordinate that. (Is
adding a new RX call a big deal?)

I'm not sure how big a deal this really is, but LockUpgrades already
seem a tad dangerous the way they are implemented - since you have a
race and the file might change between the unlock and lock if someone
else is spinning waiting for a write lock, and that becomes much more
important if you are doing byte ranges on the client, since it has to
retain the read lock.

I'd probably initially live with the race, since we already are, but
seems like implementing a LockChange subtype, or a new RX call would be
better. 

Thoughts?

-- Nathan

------------------------------------------------------------
Nathan Neulinger                       EMail:  nneul@umr.edu
University of Missouri - Rolla         Phone: (573) 341-4841
Computing Services                       Fax: (573) 341-4216