Hmmm, I'll take that bet...

    openafs(P)(U)

I'll wager that this module tries to do some getname() stuff, as that's common for ioctls.

Did they rebuild their modules when they updated to 6.5?

Yeah, looking at the upstream openafs repo here, there was some work done around a year ago to account for the rework of the getname/putname API in mainline kernels:

    http://git.openafs.org/?p=openafs.git;a=summary

My guess would be that their openafs module needs to be patched and rebuilt to account for the same change in 6.5.

(In reply to Jeff Layton from comment #5)
> Yeah, looking at the upstream openafs repo here, there was some work done
> around a year ago to account for the rework of the getname/putname API in
> mainline kernels:
> 
>     http://git.openafs.org/?p=openafs.git;a=summary
> 
> My guess would be that their openafs module needs to be patched and rebuilt
> to account for the same change in 6.5.

Thanks Jeff!

In this case it is openafs 1.6.5.1-1 being run, and this is very recent, and should include proper fixes for the changes that went into 3.7
So I'm not sure openafs is related here but it still may be at least a contributing factor.

http://git.openafs.org/?p=openafs.git;a=commit;h=331f439a25810c3031cb4edb9dcb0afae6039145

$ git log --oneline | head -1
331f439 Update NEWS for 1.6.5.1
$ git log --oneline | grep putname
c21fded Linux: change test for new putname API
cf33252 Linux: fix afs_putname wrapper for pre-3.7 kernels
5aae6e0 Linux 3.7: putname is no longer exported

...

Created attachment 833301 [details]
patch -- openafs: fix the afs_putname definition when STRUCT_FILENAME_HAS_NAME is defined

Looks like this is due to bad putname handling in the openafs code. Their code uses getname() to copy the string from userland, but then uses afs_putname to put it.

They have afs_putname() defined wrong and it's causing a double-free on the memory when auditing is enabled. This patch will likely fix it. I don't have much insight into openafs development, so feel free to pass this patch on to them if it'll help.

I tracked down the openafs-devel mailing list and sent them the patch. I'm not a subscriber to the list though, so we'll have to wait for the moderator to approve it.

Jeff,

The best option for submitting patches to OpenAFS is to use

  http://gerrit.openafs.org/

Alternatively, patches can be sent to openafs-bugs which will open a ticket in the OpenAFS Request Tracker.

Thanks.

Jeffrey Altman
OpenAFS Gatekeeper

Thanks, I'll keep that in mind for the future...

For now, I don't think that patch will help you since putname isn't exported in mainline kernels now. I'm also going to propose a patch upstream soon (once I have a chance to test it) that will unexport getname.

I think what would probably be best for openafs would be to just make a afs_getname that does a names_cache allocation, strncpy_from_user into it and return that (with proper error handling of course). It looks like all your code cares about is the string anyway. Then you can just keep afs_putname doing the kmem_cache_free and all will be well.

I'll leave this bug open for now in case you have more questions, but we'll plan to eventually close it as NOTABUG.

Thanks.

Will this break any system running the 6.5 kernel? Or is some special configuration required to trigger the problem?

Any comments on http://gerrit.openafs.org/10545 would be most welcome.

You need to have syscall auditing enabled in order to hit it. As far as the patch goes...

That new afs_getname function looks unnecessarily complicated. AFAICT, none of your callers of getname actually do anything with the struct filename. I think you just need to do a PATH_MAX allocation out of some slab (names_cachep or your own, or just kmalloc it). Then strncpy_from_user into that. Then when you do your afs_putname, free it appropriately.

At that point you'll have a kernel char * pointer that you can pass to functions that just want the name.

I also wouldn't worry about ifdef'ing any of that stuff for particular kernel versions. Just make the code unconditionally use the new routines. With that you can also get rid of afs_name_to_string() since you know that there will never be a struct filename involved.

...oh and that function seems a little large to be a static inline, doesn't it?

Created attachment 833684 [details]
patch -- stop trying to use getname/putname

Maybe something like this patch instead? Note that it prob needs cleanup -- indentation doesn't follow the openafs style for instance.

You may also want to make your own slabcache as someone in your gerrit tool mentioned.

At this point, I think the openafs folks have a handle on the problem, so I'll go ahead and close this as NOTABUG. Please reopen it if we need to discuss it further.