Bug 8974 - Kernel oplocks are broken when uid(file) != uid(process)
Summary: Kernel oplocks are broken when uid(file) != uid(process)
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: File services (show other bugs)
Version: 3.6.3
Hardware: All Linux
: P5 normal
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-03 22:39 UTC by Etienne Dechamps
Modified: 2012-06-30 11:12 UTC (History)
1 user (show)

See Also:


Attachments
Patch which fixes the issue even with a broken kernel (576 bytes, patch)
2012-06-04 19:09 UTC, Etienne Dechamps
no flags Details
git-am fx for master (1.76 KB, patch)
2012-06-15 23:27 UTC, Jeremy Allison
no flags Details
Updated fix for master. (1.75 KB, patch)
2012-06-18 21:52 UTC, Jeremy Allison
no flags Details
git-am fix. (1.76 KB, patch)
2012-06-20 22:50 UTC, Jeremy Allison
metze: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Etienne Dechamps 2012-06-03 22:39:25 UTC
With my kernel (3.2.10, Debian Unstable) Samba crashes when the following sequence of operations occurs:

 1. Samba is started with kernel oplocks = yes
 2. A file is opened whose owner UID is not the same as the Samba process which opens it 
 3. Samba tries to get a kernel lease using fcntl(F_SETSIG) then fcntl(F_SETLEASE) on the file for the kernel oplock, which fails with EACCES (as expected)
 4. Samba elevates with CAP_LEASE, then retries fcntl(F_SETLEASE), which succeeds (as expected)
 5. Something else opens the file
 6. Because of Linux kernel bug #43336 [1], Samba gets SIGIO instead of the signal it set with SETSIG (which is SIGRTMIN+1)
 7. There is no handler for SIGIO, so Samba crashes, game over.

A workaround is to disable kernel oplocks:

    kernel oplocks = no

While this is strictly speaking a kernel bug, I suggest patching Samba so that it works even with a broken kernel. Theoretically, we just need to modify the logic in source3/smbd/oplock_linux.c so that linux_set_lease_sighandler() is called before each time fcntl(F_SETLEASE) is used (i.e. in linux_setlease()).

[1] https://bugzilla.kernel.org/show_bug.cgi?id=43336
Comment 1 Etienne Dechamps 2012-06-04 19:09:22 UTC
Created attachment 7622 [details]
Patch which fixes the issue even with a broken kernel

The attached patch fixes the issue by calling SETSIG before each time SETLEASE is called, thus working around broken kernels.
Comment 2 Jeremy Allison 2012-06-15 23:27:21 UTC
Created attachment 7652 [details]
git-am fx for master

Can you test this version ? Should be identical to yours but tidies the code somewhat and also adds comments.

Jeremy.
Comment 3 Etienne Dechamps 2012-06-16 09:07:06 UTC
Your patch doesn't compile, at least on Samba 3.6.5:

    smbd/oplock_linux.c: In function ‘linux_setlease’:
    smbd/oplock_linux.c:80:32: error: ‘fsp’ undeclared (first use in this function)
Comment 4 Jeremy Allison 2012-06-18 21:52:00 UTC
Created attachment 7659 [details]
Updated fix for master.

Should compile now :-). Sorry about that.

Jeremy.
Comment 5 Etienne Dechamps 2012-06-20 17:22:08 UTC
Your updated patch seems to fix the issue.
Comment 6 Jeremy Allison 2012-06-20 18:21:34 UTC
Comment on attachment 7659 [details]
Updated fix for master.

Requesting review from metze.
Comment 7 Stefan Metzmacher 2012-06-20 19:02:28 UTC
Comment on attachment 7659 [details]
Updated fix for master.

As you asked me to review this for master, I would say please follow README.Coding :-)
Comment 8 Jeremy Allison 2012-06-20 19:04:33 UTC
Metze, I'm not in a mood to play games. If you have specific suggestions please make them.
Comment 9 Stefan Metzmacher 2012-06-20 19:13:52 UTC
Comment on attachment 7659 [details]
Updated fix for master.


>diff --git a/source3/smbd/oplock_linux.c b/source3/smbd/oplock_linux.c
>index 190578e..623a2a6 100644
>--- a/source3/smbd/oplock_linux.c
>+++ b/source3/smbd/oplock_linux.c
>@@ -76,9 +76,21 @@ int linux_setlease(int fd, int leasetype)
> {
> 	int ret;
> 
>+	/* first set the signal handler */
>+	if(linux_set_lease_sighandler(fd) == -1) {
          ^ missing whitespace

>+		return -1;
>+	}
> 	ret = fcntl(fd, F_SETLEASE, leasetype);
> 	if (ret == -1 && errno == EACCES) {
> 		set_effective_capability(LEASE_CAPABILITY);
>+		/* Bug 8974 - work around Linux kernel bug
>+		   https://bugzilla.kernel.org/show_bug.cgi?id=43336.
>+		   "fcntl(F_SETLEASE) resets signal number when
>+		    called multiple times"
>+		*/


		/*
		 * Bug 8974 - work around Linux kernel bug
		 * https://bugzilla.kernel.org/show_bug.cgi?id=43336.
		 * "fcntl(F_SETLEASE) resets signal number when
		 * called multiple times"
		 */

>+		if(linux_set_lease_sighandler(fd) == -1) {
                  ^ missing whitespace
Comment 10 Jeremy Allison 2012-06-20 22:50:28 UTC
Created attachment 7664 [details]
git-am fix.

Updated with metze's requested changes.
Comment 11 Stefan Metzmacher 2012-06-21 06:23:38 UTC
Comment on attachment 7664 [details]
git-am fix.

Looks good,

Jeremy, is this for master or also for v3-6-test and v3-5-test?

I'm fine with all of them...
Comment 12 Jeremy Allison 2012-06-21 17:10:48 UTC
Karolin please pick for 3.6.next and 3.5.next.

Thanks !

Jeremy.
Comment 13 Karolin Seeger 2012-06-30 11:12:51 UTC
Pushed to v3-6-test and v3-5-test.
Closing out bug report.

Thanks!