08 Feb, 2006

1 commit

  • I just noticed that my patch "don't create on open that fails due to
    ERR_GRACE" (recently commited as fb553c0f17444e090db951b96df4d2d71b4f4b6b)
    had an obvious problem that causes a deadlock on reboot recovery. Sending
    in this now since it seems like a clear 2.6.16 candidate.--b.

    We're returning with a lock held in some error cases.

    Signed-off-by: J. Bruce Fields
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    J. Bruce Fields
     

02 Feb, 2006

1 commit


19 Jan, 2006

24 commits

  • A recent patch which checked the return status of vfs_getattr in nfsd,
    completely missed the nfsproc.c (NFSv2) part. Here is it.

    This patch moved the call to vfs_getattr from the xdr encoding (at which point
    it is too late to return an error) to the call handling. This means several
    calls to vfs_getattr are needed in nfsproc.c. Many are encapsulated in
    nfsd_return_attrs and nfsd_return_dirop.

    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    David Shaw
     
  • nfsd_sync* return an errno, which usually needs to be converted to an errno,
    sometimes immediately, sometimes a little later.

    Also, nfsd_setattr returns an nfserr which SHOULDN'T be converted from
    an errno (because it isn't one).

    Also some tidyups of the form:
    err = XX
    err = nfserrno(err)
    and
    err = XX
    if (err)
    err = nfserrno(err)
    become
    err = nfserrno(XX)

    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    NeilBrown
     
  • missing nfserrno() in default case of a switch by return value of
    posix_lock_file(); as the result we send negative host-endian to clients that
    expect positive network-endian, preferably mentioned in RFC... BTW, that case
    is not impossible - posix_lock_file() can return -ENOLCK and we do not handle
    that one explicitly.

    Signed-off-by: Al Viro
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Al Viro
     
  • ->rp_status is network-endian and nobody byteswaps it before sending to
    client; putting NFSERR_SERVERFAULT instead of nfserr_serverfault in there is
    not nice...

    Signed-off-by: Al Viro
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Al Viro
     
  • -EINVAL (in host order, no less) is not a good thing to return to client.

    nfsd4_truncate() returns it in one case and its callers expect nfs_.... from
    it. AFAICS, it should be nfserr_inval

    Signed-off-by: Al Viro
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Al Viro
     
  • Several failure exits return -E instead of nfserr_ and
    vice versa.

    Signed-off-by: Al Viro
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Al Viro
     
  • Clean up some unnecessary special-casing in the setattr code..

    Signed-off-by: Fred Isaman
    Signed-off-by: J. Bruce Fields
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Fred Isaman
     
  • Fix bug in rdattr_error return which causes correct error code to be
    overwritten by nfserr_toosmall.

    Signed-off-by: Fred Isaman
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Fred Isaman
     
  • Bad bookkeeping of the share reservations when handling open upgrades was
    causing open downgrade to fail.

    Signed-off-by: J. Bruce Fields
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    J. Bruce Fields
     
  • In an earlier patch (commit b648330a1d741d5df8a5076b2a0a2519c69c8f41) I noted
    that a too-early grace-period check was preventing us from bumping the
    sequence id on open. Unfortunately in that patch I stupidly moved the
    grace-period check back too far, so now an open for create can succesfully
    create the file while still returning ERR_GRACE.

    The correct place for that check is after we've set the open_owner and handled
    any replays, but before we actually start mucking with the filesystem.

    Thanks to Avishay Traeger for reporting the bug.

    Signed-off-by: J. Bruce Fields
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    J. Bruce Fields
     
  • nfsd4_process_open1 is very highly nested; flatten it out a bit.

    Also, the preceding comment, which just outlines the logic, seems redundant.

    Signed-off-by: J. Bruce Fields
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    J. Bruce Fields
     
  • Remove some goto's that made the logic here a little more tortuous than
    necessary.

    Signed-off-by: J. Bruce Fields
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    J. Bruce Fields
     
  • We shouldn't check for replays until after checking whether the open owner is
    confirmed. Clients are allowed to reuse openowners without bumping the seqid.

    Signed-off-by: J. Bruce Fields
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    J. Bruce Fields
     
  • We need to make sure open reclaims are marked confirmed immediately so that we
    can handle replays even if they fail (e.g. with a seqid-incrementing error).
    (See 8.1.8.)

    Signed-off-by: J. Bruce Fields
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    J. Bruce Fields
     
  • Make sure we get a directory when we look up the recovery directory.

    Thanks to Christoph Hellwig for the bug report.

    Based on feedback from Christoph and others, we may remove the need for this
    lookup and just pass in a file descriptor from userspace instead, and/or
    completely move the directory handling to userspace. For now we're just
    fixing the obvious bugs.

    Signed-off-by: J. Bruce Fields
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    J. Bruce Fields
     
  • We should be opening this directory RDONLY, not RDWR.

    Thanks to Christoph Hellwig for the bug report.

    Signed-off-by: J. Bruce Fields
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    J. Bruce Fields
     
  • Simple, useful debugging printk: print the number of each op as we process it.

    Signed-off-by: J. Bruce Fields
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    J. Bruce Fields
     
  • Fix some bad logic.

    Signed-off-by: J. Bruce Fields
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    J. Bruce Fields
     
  • It's confusing having both release_stateowner() and release_state_owner().

    And as it turns out, release_state_owner() is short and only called from one
    place; so just remove it.

    Also note the confirmed check is superfluous there--preprocess_seqid_op
    already check this.

    And remove a redundant comment and a superfluous line assignment while we're
    at it.

    Signed-off-by: J. Bruce Fields
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    J. Bruce Fields
     
  • One of the things that's confusing about nfsd4_lock is that the lk_stateowner
    field could be set to either of two different lockowners: the open owner or
    the lock owner. Rename to lk_replay_owner and add a comment to make it clear
    that it's used for whichever stateowner has its sequence id bumped for replay
    detection.

    Signed-off-by: J. Bruce Fields
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    J. Bruce Fields
     
  • release_state_owner also puts the lock owner on the close_lru. There's no
    need for that, though; replays of the failed lock would be handled by the
    openowner not the lockowner.

    Also consolidate the cleanup a bit, fixing leaks that can happen if errors
    occur between the time a new lock owner is allocated and the lock is done.

    Remove a comment and dprintk that look a little redundant.

    Signed-off-by: J. Bruce Fields
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    J. Bruce Fields
     
  • Logic fixes for LOCK and UNLOCK.

    - Move the permission check on the current file handle outside of
    nfs4_lock_state()

    - remove the file manager fl_release_private calls; fl_ops is not set.

    Signed-off-by: Andy Adamson
    Signed-off-by: J. Bruce Fields
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Andy Adamson
     
  • These are both called from two places close together. I could rearrange that
    code so there is only one call site, but just removing the 'inline' is
    probably best.

    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    NeilBrown
     
  • Change nfsd_sync_dir to return an error if ->sync fails, and pass that error
    up through the stack. This involves a number of rearrangements of error
    paths, and care to distinguish between Linux -errno numbers and NFSERR
    numbers.

    In the 'create' routines, we continue with the 'setattr' even if a previous
    sync_dir failed.

    This patch is quite different from Takashi's in a few ways, but there is still
    a strong lineage.

    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    YAMAMOTO Takashi
     

15 Jan, 2006

1 commit


11 Jan, 2006

1 commit

  • Add vfs_getxattr, vfs_setxattr and vfs_removexattr helpers for common checks
    around invocation of the xattr methods. NFSD already was missing some of the
    checks and there will be more soon.

    Signed-off-by: Christoph Hellwig
    Cc: James Morris

    (James, I haven't touched selinux yet because it's doing various odd things
    and I'm not sure how it would interact with the security attribute fallbacks
    you added. Could you investigate whether it could use vfs_getxattr or if not
    add a __vfs_getxattr helper to share the bits it is fine with?)

    For NFSv4: instead of just converting it add an nfsd_getxattr helper for the
    code shared by NFSv2/3 and NFSv4 ACLs. In fact that code isn't even
    NFS-specific, but I'll wait for more users to pop up first before moving it to
    common code.

    Signed-off-by: Christoph Hellwig
    Acked-by: Dave Kleikamp
    Signed-off-by: Adrian Bunk
    Signed-off-by: Neil Brown
    Cc: Trond Myklebust
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Christoph Hellwig
     

10 Jan, 2006

1 commit


07 Jan, 2006

4 commits

  • Clean up: Every ULP that uses the in-kernel RPC client, except the NLM
    client, sets cl_chatty. There's no reason why NLM shouldn't set it, so
    just get rid of cl_chatty and always be verbose.

    Test-plan:
    Compile with CONFIG_NFS enabled.

    Signed-off-by: Chuck Lever
    Signed-off-by: Trond Myklebust

    Chuck Lever
     
  • Shrink the RPC task structure. Instead of storing separate pointers
    for task->tk_exit and task->tk_release, put them in a structure.

    Also pass the user data pointer as a parameter instead of passing it via
    task->tk_calldata. This enables us to nest callbacks.

    Signed-off-by: Trond Myklebust

    Trond Myklebust
     
  • A typical nfsd call trace is
    nfsd -> svc_process -> nfsd_dispatch -> nfsd3_proc_write ->
    nfsd_write ->nfsd_vfs_write -> vfs_writev

    These add up to over 300 bytes on the stack.
    Looking at each of these, I see that nfsd_write (which includes
    nfsd_vfs_write) contributes 0x8c to stack usage itself!!

    It turns out this is because it puts a 'struct iattr' on the stack so
    it can kill suid if needed. The following patch saves about 50 bytes
    off the stack in this call path.

    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Neil Brown
     
  • Both vfs_getattr and i_op->fsync return error statuses which nfsd was
    largely ignoring. This as noticed when exporting directories using fuse.

    This patch cleans up most of the offences, which involves moving the call
    to vfs_getattr out of the xdr encoding routines (where it is too late to
    report an error) into the main NFS procedure handling routines.

    There is still a called to vfs_gettattr (related to the ACL code) where the
    status is ignored, and called to nfsd_sync_dir don't check return status
    either.

    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    David Shaw
     

21 Dec, 2005

1 commit


07 Nov, 2005

6 commits

  • This is the fs/ part of the big kfree cleanup patch.

    Remove pointless checks for NULL prior to calling kfree() in fs/.

    Signed-off-by: Jesper Juhl
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Jesper Juhl
     
  • If an RPC socket is serving multiple programs, then the pg_authenticate of
    the first program in the list is called, instead of pg_authenticate for the
    program to be run.

    This does not cause a problem with any programs in the current kernel, but
    could confuse future code.

    Also set pg_authenticate for nfsd_acl_program incase it ever gets used.

    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    NeilBrown
     
  • There are a couple of tests which could possibly be confused by extremely
    large numbers appearing in 'xdr' packets. I think the closest to an exploit
    you could get would be writing random data from a free page into a file - i.e.
    leak data out of kernel space.

    I'm fairly sure they cannot be used for remote compromise.

    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    NeilBrown
     
  • Provide a file in the NFSD filesystem that allows setting and querying of
    which version of NFS are being exported. Changes are only allowed while no
    server is running.

    Signed-off-by: Steve Dickson
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    NeilBrown
     
  • Most files in the nfsd filesystems are transaction files. You write a
    request, and read a response.

    For some (e.g. 'threads') it makes sense to just be able to read and get the
    current value.

    This functionality did exist but was broken recently when someone modified
    nfsctl.c without going through the maintainer. This patch fixes the
    regression.

    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    NeilBrown
     
  • This is a somewhat cosmetic fix to keep the SpecFS validation test from
    complaining.

    SpecFS want's to try chmod on symlinks, and ext3 and reiser (at least) return
    ENOTSUPP.

    Probably both sides are being silly, but it is easiest to simply make it a
    non-issue and filter out chmod requests on symlinks at the nfsd level.

    Signed-off-by: Olaf Kirch
    Signed-off-by: J. Bruce Fields
    Signed-off-by: Neil Brown
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    NeilBrown