Re: preconditions of cache_enter

[Antti Kantee <pooka%cs.hut.fi@localhost>]

On Fri Jun 11 2010 at 05:16:50 +0000, Taylor R Campbell wrote:
> (I am not subscribed to this list, so please cc me in replies.)
> 
> cache_enter assumes that cnp->cn_namelen is at most NCHNAMLEN.[*]  Most
> callers first call cache_lookup, which clears the MAKEENTRY flag if
> the name is too long.  Some neither call cache_lookup nor check the
> name length first, however.  I have observed a panic in puffs (without
> harm to my system, thanks to rump_syspuffs) from cache_enter while
> using long file names.
> 
> As far as I can tell from a cursory reading of the code, puffs_newnode
> calls cache_enter without first calling cache_lookup and without
> checking the name length, and the same goes for all four of its
> callers (puffs_vnop_create, puffs_vnop_mknod, puffs_vnop_mkdir, and
> puffs_vnop_symlink).  I believe the same goes for smbfs_create.

cache_lookup() is called in puffs_vnop_lookup().  The same componentname
is then passed to the rest of the operations.  The atomicity of this set
of operations is guaranteed by taking an exclusive lock on the directory.

I remember working with long pathname components when i was doing
measurements for the ReFUSE paper, but that was, hmm, about 3 years ago,
so some bug might have snuck up after that.

> I don't know this code well enough to suggest a patch, I'm afraid.  It
> would be easy to change all the callers of cache_enter that are not
> protected by a preceding cache_lookup to check the name length
> themselves, but that may be an abstraction violation.  What is the
> right thing to do here?  (File a PR?)

Please send more details about the crash (on list or directly to me).

Coincidentally, I was planning to work on puffs today (unless I get
distracted by the way shinier regression tests).

thanks,
  antti