On Sun, 2012-07-22 at 17:26 +0900, sf...@users.sourceforge.net wrote: > Hello Ben, > > Ben Hutchings: > > What are these changes supposed to do? > > > > commit aa29a06fd0c6113560f2c5e3e9630fe67b097ba7 > > Author: J. R. Okajima <hooano...@yahoo.co.jp> > > Date: Tue Jun 12 22:08:17 2012 +0900 > > > > aufs: for 3.5-rcN, new au_security_file_mmap() 1/2 > > > > commit ad02614f68494af290af54325895091917a6b7c7 > > Author: J. R. Okajima <hooano...@yahoo.co.jp> > > Date: Tue Jun 12 22:08:39 2012 +0900 > > > > aufs: for 3.5-rcN, call au_security_file_mmap() 2/2 > > Isn't the aa29a06 commit log enough for you? > > > An earlier change for 3.5 added this comment, which was removed by the > > second commit above: > > > > /* todo: the locking order between mmap_sem */ > > /* > > * err =3D security_mmap_file(h_file, au_prot_conv(vma->vm_flags), > > * au_flag_conv(vma->vm_flags)); > > */ > > If you looked the another commit (a1d9363), the comments just before > aufs_mmap() (below), and the release mails from me (below), then you might > know > what these commits for are.
I'm sorry - how could I have missed these comments?! I was concentrating too much on the patches and not the context, I suppose. > > > Did lockdep report that an mmap_sem and some other lock (needed by an > > LSM) were not taken in the usual order? If so, moving the > > security_mmap_file() into another thread (workqueue) won't solve the > > possible deadlock, though it may well hide it from lockdep. > > Before linux-3.5, aufs called security_file_mmap() called directly. > In 3.5, security_file_mmap() is splitted into two parts as > - security_mmap_addr(), which is called BEFORE acquiring mmap_sem and > checks the mapping address. > - security_mmap_file(), which is called AFTER acquiring mmap_sem and > checks the mapping file. security_mmap_addr() is called with mmap_sem held, but anyway we don't need to call it from aufs... > Since security_mmap_file() explicitly expects mmap_sem held, I moved > the security_mmap_file() call to kworker. I don't know whether > security_mmap_file() actulally causes a deadlock or not. > This is very dirty approach (as I wrote), but it is effective. Because > mmap_sem is an object per task (process or thread), the deadlock will > never happen. The mmap_sem for the process is held, but the mmap_sem for > the kworker is not (and it is unrelated). > Don't you agree? Yes, I think you have avoided the potential deadlock, though I'm not sure. > (aa29a06 commit) > ---------------------------------------------------------------------- > A bad approach in order to just call security_file_mmap() for the > real file on branch fs from outside of 'mmap_sem'-protected region. > It may be meaningless since it is done in kworker context. > I really don't like this approach but I don't have another approach to > call security_file_mmap(). [...] Well, now security_mmap_file() may use wrong permissions, log wrong audit messages, or kill the wrong task (the workqueue task). This is much worse than the potential deadlock (which I don't believe will be triggered by any current LSM). Ben. -- Ben Hutchings 73.46% of all statistics are made up.
signature.asc
Description: This is a digitally signed message part
------------------------------------------------------------------------------ Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
