✗ travis-ci: failure for series starting with [PATCHv2,1/5] locks: Add c/r of non broken leases (kernel>=v4.1)

Submitted by Andrei Vagin on Sept. 4, 2017, 9:12 p.m.

Details

Message ID 20170904211237.GB19707@outlook.office365.com
State Accepted
Series "Series without cover letter"
Headers show

Commit Message

Andrei Vagin Sept. 4, 2017, 9:12 p.m.
On Sun, Sep 03, 2017 at 05:36:58PM +0300, Pavel Begunkov wrote:
> Seems, CRIU can't set a lease on a file when user namespaces are used.
> So, I found a notice from *fcntl* man:
> 
> "An unprivileged process may take out a lease only on a file whose UID
> (owner) matches the filesystem UID of the process.  A process with the
> CAP_LEASE capability may take out leases on arbitrary files."
> 
> I suppose, that restore stage is performed in user namespace and,
> because of that, filesystem UID of new process differ from the original
> one. Thus, it leads to violation of statements above.
> 
> Could somebody with a better understanding of namespaces in CRIU tell if
> it sounds reasonable and guess what I can do about it, please?

It is possible to use something like this:

        int ret;
@@ -523,9 +525,17 @@ static int restore_file_lease(FileLockEntry *fle)
        if (fle->type & LEASE_BREAKING) {
                return restore_breaking_file_lease(fle);
        } else {
+               struct stat st;
+               int old_fsuid;
+               if (fstat(fle->fd, &st)) {
+                       pr_perror("stat");
+                       return 1;
+               }
+               old_fsuid = setfsuid(st.st_uid);
                ret = fcntl(fle->fd, F_SETLEASE, fle->type);
                if (ret < 0)
                        pr_perror("Can't restore non broken lease");
+               setfsuid(old_fsuid);
                return ret;
        }
 }


And if this way will fail too, we can create a slow path, which will
work via the usernsd daemon. Usernsd is running in the root user
namespaces with all capabilities, and it is possible to ask it to
set a lease. You need to look at userns_call().

> 
> 
> On 30/08/17 04:04, Pavel Begunkov wrote:
> > Yeah, thanks. Soon I'll make time to fix it along with one's noticed by
> > Pavel.
> > 
> > On 30/08/17 02:43, Andrei Vagin wrote:
> >> ===================== Run zdtm/static/file_lease01 in uns ======================
> >> Start test
> >> ./file_lease01 --pidfile=file_lease01.pid --outfile=file_lease01.out --filename=file_lease01.test
> >> Run criu dump
> >> Run criu restore
> >> =[log]=> dump/zdtm/static/file_lease01/124/1/restore.log
> >> ------------------------ grep Error ------------------------
> >> (00.308462)      4: 		Create fd for 3
> >> (00.308480)      4: 		Create fd for 4
> >> (00.308486)      4: 		Create fd for 5
> >> (00.308491)      4: 		Create fd for 6
> >> (00.308510)      4: Error (criu/file-lock.c:528): Can't restore non broken lease: Permission denied
> >> (00.310868)      1: Error (criu/cr-restore.c:1527): 4 exited, status=1
> >> (00.346609) uns: calling exit_usernsd (-1, 1)
> >> (00.346678) uns: daemon calls 0x4d3390 (146, -1, 1)
> >> (00.346690) uns: `- daemon exits w/ 0
> >> (00.349075) uns: daemon stopped
> >> (00.349092) Error (criu/cr-restore.c:2412): Restoring FAILED.
> >> ------------------------ ERROR OVER ------------------------
> >> ############## Test zdtm/static/file_lease01 FAIL at CRIU restore ##############
> >>
> >> On Mon, Aug 14, 2017 at 01:12:36AM +0000, Patchwork wrote:
> >>> == Series Details ==
> >>>
> >>> Series: series starting with [PATCHv2,1/5] locks: Add c/r of non broken leases (kernel>=v4.1)
> >>> URL   : https://patchwork.criu.org/series/1885/
> >>> State : failure
> >>>
> >>> == Logs ==
> >>>
> >>> For more details see: https://travis-ci.org/criupatchwork/criu/builds/264198066?utm_source=github_status&utm_medium=notification
> >>> _______________________________________________
> >>> CRIU mailing list
> >>> CRIU@openvz.org
> >>> https://lists.openvz.org/mailman/listinfo/criu
> > 
> 
> -- 
> Yours sincerely,
> Pavel
>

Patch hide | download patch | download mbox

diff --git a/criu/file-lock.c b/criu/file-lock.c
index c2db04497..1b75fdfdc 100644
--- a/criu/file-lock.c
+++ b/criu/file-lock.c
@@ -516,6 +516,8 @@  static int restore_breaking_file_lease(FileLockEntry
*fle)
        return 0;
 }
 
+int setfsuid(uid_t fsuid);
+int setfsgid(gid_t fsuid);
 static int restore_file_lease(FileLockEntry *fle)
 {

Comments

Pavel Begunkov (Silence) Sept. 7, 2017, 8:28 a.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Thanks, it worked out locally.

I just wonder if it's appropriate, that process will be unable to set
lease after restoring, even if it was possible before. If it's up to
user, maybe we shouldn't workaround it and just fail during restoring?

On 09/05/2017 12:12 AM, Andrei Vagin wrote:
> On Sun, Sep 03, 2017 at 05:36:58PM +0300, Pavel Begunkov wrote:
>> Seems, CRIU can't set a lease on a file when user namespaces are used
.
>> So, I found a notice from *fcntl* man:
>>
>> "An unprivileged process may take out a lease only on a file whose UI
D
>> (owner) matches the filesystem UID of the process.  A process with th
e
>> CAP_LEASE capability may take out leases on arbitrary files."
>>
>> I suppose, that restore stage is performed in user namespace and,
>> because of that, filesystem UID of new process differ from the origin
al
>> one. Thus, it leads to violation of statements above.
>>
>> Could somebody with a better understanding of namespaces in CRIU tell
 if
>> it sounds reasonable and guess what I can do about it, please?
> 
> It is possible to use something like this:
> 
> diff --git a/criu/file-lock.c b/criu/file-lock.c
> index c2db04497..1b75fdfdc 100644
> --- a/criu/file-lock.c
> +++ b/criu/file-lock.c
> @@ -516,6 +516,8 @@ static int restore_breaking_file_lease(FileLockEnt
ry
> *fle)
>         return 0;
>  }
>  
> +int setfsuid(uid_t fsuid);
> +int setfsgid(gid_t fsuid);
>  static int restore_file_lease(FileLockEntry *fle)
>  {
>         int ret;
> @@ -523,9 +525,17 @@ static int restore_file_lease(FileLockEntry *fle)
>         if (fle->type & LEASE_BREAKING) {
>                 return restore_breaking_file_lease(fle);
>         } else {
> +               struct stat st;
> +               int old_fsuid;
> +               if (fstat(fle->fd, &st)) {
> +                       pr_perror("stat");
> +                       return 1;
> +               }
> +               old_fsuid = setfsuid(st.st_uid);
>                 ret = fcntl(fle->fd, F_SETLEASE, fle->type);
>                 if (ret < 0)
>                         pr_perror("Can't restore non broken lease");
> +               setfsuid(old_fsuid);
>                 return ret;
>         }
>  }
> 
> 
> And if this way will fail too, we can create a slow path, which will
> work via the usernsd daemon. Usernsd is running in the root user
> namespaces with all capabilities, and it is possible to ask it to
> set a lease. You need to look at userns_call().
> 
>>
>>
>> On 30/08/17 04:04, Pavel Begunkov wrote:
>>> Yeah, thanks. Soon I'll make time to fix it along with one's noticed
 by
>>> Pavel.
>>>
>>> On 30/08/17 02:43, Andrei Vagin wrote:
>>>> ===================== Run zdtm/static/file_lease01 in uns =========
=============
>>>> Start test
>>>> ./file_lease01 --pidfile=file_lease01.pid --outfile=file_lease01.ou
t --filename=file_lease01.test
>>>> Run criu dump
>>>> Run criu restore
>>>> =[log]=> dump/zdtm/static/file_lease01/124/1/restore.log
>>>> ------------------------ grep Error ------------------------
>>>> (00.308462)      4: 		Create fd for 3
>>>> (00.308480)      4: 		Create fd for 4
>>>> (00.308486)      4: 		Create fd for 5
>>>> (00.308491)      4: 		Create fd for 6
>>>> (00.308510)      4: Error (criu/file-lock.c:528): Can't restore non
 broken lease: Permission denied
>>>> (00.310868)      1: Error (criu/cr-restore.c:1527): 4 exited, statu
s=1
>>>> (00.346609) uns: calling exit_usernsd (-1, 1)
>>>> (00.346678) uns: daemon calls 0x4d3390 (146, -1, 1)
>>>> (00.346690) uns: `- daemon exits w/ 0
>>>> (00.349075) uns: daemon stopped
>>>> (00.349092) Error (criu/cr-restore.c:2412): Restoring FAILED.
>>>> ------------------------ ERROR OVER ------------------------
>>>> ############## Test zdtm/static/file_lease01 FAIL at CRIU restore #
#############
>>>>
>>>> On Mon, Aug 14, 2017 at 01:12:36AM +0000, Patchwork wrote:
>>>>> == Series Details ==
>>>>>
>>>>> Series: series starting with [PATCHv2,1/5] locks: Add c/r of non b
roken leases (kernel>=v4.1)
>>>>> URL   : https://patchwork.criu.org/series/1885/
>>>>> State : failure
>>>>>
>>>>> == Logs ==
>>>>>
>>>>> For more details see: https://travis-ci.org/criupatchwork/criu/bui
lds/264198066?utm_source=github_status&utm_medium=notification
>>>>> _______________________________________________
>>>>> CRIU mailing list
>>>>> CRIU@openvz.org
>>>>> https://lists.openvz.org/mailman/listinfo/criu
>>>
>>
>> -- 
>> Yours sincerely,
>> Pavel
>>
> 
> 
> 

- -- 
Best regards,
Pavel
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE+6JuPTjTbx479o3OWt5b1Glr+6UFAlmxAxgACgkQWt5b1Glr
+6Ubpw/+MEJ2nL/MJiSFhFFgTIXgn6HnLEUKg0kp2xFJubIm07HP81KYsM37AL2r
xg4QxSH/jkZVgnMDquAS2hDOj6oyC9sKTZLotxESXeGQJgIVA+zNFJGFwd7P3btl
zdD+DCMCtk//XOhOOi6bpH0P4PtkdTuvtu6TecEwczrpxqtI0K0csNOhrUNy+Hg9
VyCadIIFI8uPTxpaUHm+zfQkj9Ll+tUHc+ewFIxlip53LHqDThVzEK8nquu82ckV
hwANKBxfMaCPOCw7bRLfGzSgH64g967kIG1iUih8SDDjfzdaZmLcHe38vps8SUlc
snV71PCAgJvQnxGGd/O1VHHxnTuqJXOHDe42iQHNYfHJtF5y1NcY6AUOiMqP8zya
Nbv3wMOuyw4zLc0phbkCIuf6S/J2510JXUThC9s2VC96E+v5+bCM4HE2ci+NcYzV
8FoVBQvflZzQBNjpgpwnUN8RpjNlcbkNq0TP6lTB7oY5ZGTsuHL/Pk0abXUtkEkD
EpUvZtgml8aqjvrS6QtvaRjwlVkkfzulCOJGJHPvQ6/uywklkPumElFC2CSsgT0Z
jJhNt0wOIvK3WaQRI4vqHFBwE4QqAsDIQ2Urz2kYP/NjCqUTzO7bwHaqVVOOqjOl
Y2wdQ5Ih7McY5JqBkOBwNeUsqVQRxt9au95EM83HCwK2EJScg9s=
=mk5p
-----END PGP SIGNATURE-----