sk-unix: set mntinfo

Submitted by Christian Brauner on Sept. 7, 2016, 2:29 p.m.

Details

Message ID 20160907142948.25492-2-christian.brauner@canonical.com
State Rejected
Series "sk-unix: set mntinfo"
Headers show

Commit Message

Christian Brauner Sept. 7, 2016, 2:29 p.m.
phys_stat_resolve() call mount_resolve_path() which requires that mntinfo_tree
in the ns_id struct is initialized. This is a problem we observed with sockets
on btrfs volumes:

 Program received signal SIGSEGV, Segmentation fault.
 0x00005555555bb6dd in mount_resolve_path (mntinfo_tree=<optimized out>, path=0x555555875790 "/var/lib/lxd/unix.socket") at criu/mount.c:213
 213     criu/mount.c: No such file or directory.
 (gdb) bt
 #0  0x00005555555bb6dd in mount_resolve_path (mntinfo_tree=<optimized out>, path=0x555555875790 "/var/lib/lxd/unix.socket") at criu/mount.c:213
 #1  0x00005555555be240 in phys_stat_resolve_dev (ns=<optimized out>, st_dev=43, path=<optimized out>) at criu/mount.c:240
 #2  0x00005555555be2bb in phys_stat_dev_match (st_dev=<optimized out>, phys_dev=41, ns=ns@entry=0x5555558753a0,
     path=path@entry=0x555555875790 "/var/lib/lxd/unix.socket") at criu/mount.c:256
 #3  0x00005555555e75ed in unix_process_name (d=d@entry=0x5555558756e0, tb=tb@entry=0x7fffffffe0c0, m=<optimized out>) at criu/sk-unix.c:565
 #4  0x00005555555e9378 in unix_collect_one (tb=0x7fffffffe0c0, m=0x555555869f18 <buf+312>) at criu/sk-unix.c:620
 #5  unix_receive_one (h=0x555555869f08 <buf+296>, arg=<optimized out>) at criu/sk-unix.c:692
 #6  0x00005555555b85aa in nlmsg_receive (buf=<optimized out>, arg=<optimized out>, err_cb=<optimized out>, cb=<optimized out>, len=<optimized out>)
     at criu/libnetlink.c:45
 #7  do_rtnl_req (nl=nl@entry=5, req=req@entry=0x7fffffffe220, size=size@entry=72, receive_callback=0x5555555e9290 <unix_receive_one>,
     error_callback=0x5555555b83d0 <rtnl_return_err>, error_callback@entry=0x0, arg=arg@entry=0x0) at criu/libnetlink.c:119
 #8  0x00005555555e9cf7 in do_collect_req (nl=nl@entry=5, req=req@entry=0x7fffffffe220, receive_callback=<optimized out>, arg=arg@entry=0x0, size=72)
     at criu/sockets.c:610
 #9  0x00005555555eb1d0 in collect_sockets (ns=ns@entry=0x7fffffffe300) at criu/sockets.c:636
 #10 0x000055555559ddfc in check_sock_diag () at criu/cr-check.c:118
 #11 cr_check () at criu/cr-check.c:999
 #12 0x00005555555872d0 in main (argc=<optimized out>, argv=0x7fffffffe678, envp=<optimized out>) at criu/crtools.c:719

Signed-off-by: Christian Brauner <christian.brauner@canonical.com>
---
 criu/sk-unix.c | 1 +
 1 file changed, 1 insertion(+)

Patch hide | download patch | download mbox

diff --git a/criu/sk-unix.c b/criu/sk-unix.c
index 99f0b08..a199ca4 100644
--- a/criu/sk-unix.c
+++ b/criu/sk-unix.c
@@ -525,6 +525,7 @@  static int unix_process_name(struct unix_sk_desc *d, const struct unix_diag_msg
 			ret = -ENOENT;
 			goto out;
 		}
+		ns->mnt.mntinfo_tree = mntinfo;
 
 		mntns_root = mntns_get_root_fd(ns);
 		if (mntns_root < 0) {

Comments

Pavel Emelianov Sept. 13, 2016, 2:53 p.m.
On 09/07/2016 05:29 PM, Christian Brauner wrote:
> phys_stat_resolve() call mount_resolve_path() which requires that mntinfo_tree
> in the ns_id struct is initialized. This is a problem we observed with sockets
> on btrfs volumes:

Sorry for the late response. See my comment inline.

> Signed-off-by: Christian Brauner <christian.brauner@canonical.com>
> ---
>  criu/sk-unix.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/criu/sk-unix.c b/criu/sk-unix.c
> index 99f0b08..a199ca4 100644
> --- a/criu/sk-unix.c
> +++ b/criu/sk-unix.c
> @@ -525,6 +525,7 @@ static int unix_process_name(struct unix_sk_desc *d, const struct unix_diag_msg
>  			ret = -ENOENT;
>  			goto out;
>  		}
> +		ns->mnt.mntinfo_tree = mntinfo;

I guess that's too late for this assignment. Since this is dump time, then
ns->mnt.mntinfo_tree should have been initialized in collect_mntns() or
nearby.

>  		mntns_root = mntns_get_root_fd(ns);
>  		if (mntns_root < 0) {
>
Christian Brauner Sept. 13, 2016, 8:51 p.m.
On Tue, Sep 13, 2016 at 05:53:59PM +0300, Pavel Emelyanov wrote:
> On 09/07/2016 05:29 PM, Christian Brauner wrote:
> > phys_stat_resolve() call mount_resolve_path() which requires that mntinfo_tree
> > in the ns_id struct is initialized. This is a problem we observed with sockets
> > on btrfs volumes:
> 
> Sorry for the late response. See my comment inline.
> 
> > Signed-off-by: Christian Brauner <christian.brauner@canonical.com>
> > ---
> >  criu/sk-unix.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/criu/sk-unix.c b/criu/sk-unix.c
> > index 99f0b08..a199ca4 100644
> > --- a/criu/sk-unix.c
> > +++ b/criu/sk-unix.c
> > @@ -525,6 +525,7 @@ static int unix_process_name(struct unix_sk_desc *d, const struct unix_diag_msg
> >  			ret = -ENOENT;
> >  			goto out;
> >  		}
> > +		ns->mnt.mntinfo_tree = mntinfo;
> 
> I guess that's too late for this assignment. Since this is dump time, then
> ns->mnt.mntinfo_tree should have been initialized in collect_mntns() or
> nearby.

It is in collect_mntinfo() but it seems calling lookup_ns_by_id() in
unix_process_name() does return an ns_id but ns.mnt->mntinfo_tree is not
initialized. Is mntinfo a global that is identical for all ns_ids in the linked
list? If not how should they be initialized?

> 
> >  		mntns_root = mntns_get_root_fd(ns);
> >  		if (mntns_root < 0) {
> > 
>
Christian Brauner Sept. 13, 2016, 9:13 p.m.
On Tue, Sep 13, 2016 at 10:51:04PM +0200, Christian Brauner wrote:
> On Tue, Sep 13, 2016 at 05:53:59PM +0300, Pavel Emelyanov wrote:
> > On 09/07/2016 05:29 PM, Christian Brauner wrote:
> > > phys_stat_resolve() call mount_resolve_path() which requires that mntinfo_tree
> > > in the ns_id struct is initialized. This is a problem we observed with sockets
> > > on btrfs volumes:
> > 
> > Sorry for the late response. See my comment inline.
> > 
> > > Signed-off-by: Christian Brauner <christian.brauner@canonical.com>
> > > ---
> > >  criu/sk-unix.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/criu/sk-unix.c b/criu/sk-unix.c
> > > index 99f0b08..a199ca4 100644
> > > --- a/criu/sk-unix.c
> > > +++ b/criu/sk-unix.c
> > > @@ -525,6 +525,7 @@ static int unix_process_name(struct unix_sk_desc *d, const struct unix_diag_msg
> > >  			ret = -ENOENT;
> > >  			goto out;
> > >  		}
> > > +		ns->mnt.mntinfo_tree = mntinfo;
> > 
> > I guess that's too late for this assignment. Since this is dump time, then
> > ns->mnt.mntinfo_tree should have been initialized in collect_mntns() or
> > nearby.
> 
> It is in collect_mntinfo() but it seems calling lookup_ns_by_id() in
> unix_process_name() does return an ns_id but ns.mnt->mntinfo_tree is not
> initialized. Is mntinfo a global that is identical for all ns_ids in the linked
> list? If not how should they be initialized?

Scratch that, collect_mntinfo() is per namespace. The question is when to call
it. One option is that as soon as you add a new ns_id you call collect_mntinfo()
or you call it *first* when you need it, i.e. you check whether mntinfo_tree ==
NULL and if so call collect_mntinfo() on the corresponding ns_id.

> 
> > 
> > >  		mntns_root = mntns_get_root_fd(ns);
> > >  		if (mntns_root < 0) {
> > > 
> >
Pavel Emelianov Sept. 15, 2016, 11:07 a.m.
On 09/14/2016 12:13 AM, Christian Brauner wrote:
> On Tue, Sep 13, 2016 at 10:51:04PM +0200, Christian Brauner wrote:
>> On Tue, Sep 13, 2016 at 05:53:59PM +0300, Pavel Emelyanov wrote:
>>> On 09/07/2016 05:29 PM, Christian Brauner wrote:
>>>> phys_stat_resolve() call mount_resolve_path() which requires that mntinfo_tree
>>>> in the ns_id struct is initialized. This is a problem we observed with sockets
>>>> on btrfs volumes:
>>>
>>> Sorry for the late response. See my comment inline.
>>>
>>>> Signed-off-by: Christian Brauner <christian.brauner@canonical.com>
>>>> ---
>>>>  criu/sk-unix.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/criu/sk-unix.c b/criu/sk-unix.c
>>>> index 99f0b08..a199ca4 100644
>>>> --- a/criu/sk-unix.c
>>>> +++ b/criu/sk-unix.c
>>>> @@ -525,6 +525,7 @@ static int unix_process_name(struct unix_sk_desc *d, const struct unix_diag_msg
>>>>  			ret = -ENOENT;
>>>>  			goto out;
>>>>  		}
>>>> +		ns->mnt.mntinfo_tree = mntinfo;
>>>
>>> I guess that's too late for this assignment. Since this is dump time, then
>>> ns->mnt.mntinfo_tree should have been initialized in collect_mntns() or
>>> nearby.
>>
>> It is in collect_mntinfo() but it seems calling lookup_ns_by_id() in
>> unix_process_name() does return an ns_id but ns.mnt->mntinfo_tree is not
>> initialized. Is mntinfo a global that is identical for all ns_ids in the linked
>> list? If not how should they be initialized?
> 
> Scratch that, collect_mntinfo() is per namespace. The question is when to call
> it. One option is that as soon as you add a new ns_id you call collect_mntinfo()
> or you call it *first* when you need it, i.e. you check whether mntinfo_tree ==
> NULL and if so call collect_mntinfo() on the corresponding ns_id.

Actually collect_mntinfo() is called for every namespace found already. Can you
trace back why the ns you see isn't yet collect_mntinfo()-ed at the time the
unix_process_name() is?

-- Pavel
Christian Brauner Sept. 16, 2016, 11:19 p.m.
On Thu, Sep 15, 2016 at 02:07:47PM +0300, Pavel Emelyanov wrote:
> On 09/14/2016 12:13 AM, Christian Brauner wrote:
> > On Tue, Sep 13, 2016 at 10:51:04PM +0200, Christian Brauner wrote:
> >> On Tue, Sep 13, 2016 at 05:53:59PM +0300, Pavel Emelyanov wrote:
> >>> On 09/07/2016 05:29 PM, Christian Brauner wrote:
> >>>> phys_stat_resolve() call mount_resolve_path() which requires that mntinfo_tree
> >>>> in the ns_id struct is initialized. This is a problem we observed with sockets
> >>>> on btrfs volumes:
> >>>
> >>> Sorry for the late response. See my comment inline.
> >>>
> >>>> Signed-off-by: Christian Brauner <christian.brauner@canonical.com>
> >>>> ---
> >>>>  criu/sk-unix.c | 1 +
> >>>>  1 file changed, 1 insertion(+)
> >>>>
> >>>> diff --git a/criu/sk-unix.c b/criu/sk-unix.c
> >>>> index 99f0b08..a199ca4 100644
> >>>> --- a/criu/sk-unix.c
> >>>> +++ b/criu/sk-unix.c
> >>>> @@ -525,6 +525,7 @@ static int unix_process_name(struct unix_sk_desc *d, const struct unix_diag_msg
> >>>>  			ret = -ENOENT;
> >>>>  			goto out;
> >>>>  		}
> >>>> +		ns->mnt.mntinfo_tree = mntinfo;
> >>>
> >>> I guess that's too late for this assignment. Since this is dump time, then
> >>> ns->mnt.mntinfo_tree should have been initialized in collect_mntns() or
> >>> nearby.
> >>
> >> It is in collect_mntinfo() but it seems calling lookup_ns_by_id() in
> >> unix_process_name() does return an ns_id but ns.mnt->mntinfo_tree is not
> >> initialized. Is mntinfo a global that is identical for all ns_ids in the linked
> >> list? If not how should they be initialized?
> > 
> > Scratch that, collect_mntinfo() is per namespace. The question is when to call
> > it. One option is that as soon as you add a new ns_id you call collect_mntinfo()
> > or you call it *first* when you need it, i.e. you check whether mntinfo_tree ==
> > NULL and if so call collect_mntinfo() on the corresponding ns_id.
> 
> Actually collect_mntinfo() is called for every namespace found already. Can you
> trace back why the ns you see isn't yet collect_mntinfo()-ed at the time the
> unix_process_name() is?

This mainly affects cr-check.c and there only collect_mntinfo() is called. I'd
argue that when unix_process_name() is called and calls lookup_ns_by_id() a
namespace is retrieved for which we did not yet collect_mntinfo() in cr-check.c.
So one possibility is to use collect_mnt_namespaces() instead of
collect_mntinfo().
Pavel Emelianov Sept. 19, 2016, 9:24 a.m.
On 09/17/2016 02:19 AM, Christian Brauner wrote:
> On Thu, Sep 15, 2016 at 02:07:47PM +0300, Pavel Emelyanov wrote:
>> On 09/14/2016 12:13 AM, Christian Brauner wrote:
>>> On Tue, Sep 13, 2016 at 10:51:04PM +0200, Christian Brauner wrote:
>>>> On Tue, Sep 13, 2016 at 05:53:59PM +0300, Pavel Emelyanov wrote:
>>>>> On 09/07/2016 05:29 PM, Christian Brauner wrote:
>>>>>> phys_stat_resolve() call mount_resolve_path() which requires that mntinfo_tree
>>>>>> in the ns_id struct is initialized. This is a problem we observed with sockets
>>>>>> on btrfs volumes:
>>>>>
>>>>> Sorry for the late response. See my comment inline.
>>>>>
>>>>>> Signed-off-by: Christian Brauner <christian.brauner@canonical.com>
>>>>>> ---
>>>>>>  criu/sk-unix.c | 1 +
>>>>>>  1 file changed, 1 insertion(+)
>>>>>>
>>>>>> diff --git a/criu/sk-unix.c b/criu/sk-unix.c
>>>>>> index 99f0b08..a199ca4 100644
>>>>>> --- a/criu/sk-unix.c
>>>>>> +++ b/criu/sk-unix.c
>>>>>> @@ -525,6 +525,7 @@ static int unix_process_name(struct unix_sk_desc *d, const struct unix_diag_msg
>>>>>>  			ret = -ENOENT;
>>>>>>  			goto out;
>>>>>>  		}
>>>>>> +		ns->mnt.mntinfo_tree = mntinfo;
>>>>>
>>>>> I guess that's too late for this assignment. Since this is dump time, then
>>>>> ns->mnt.mntinfo_tree should have been initialized in collect_mntns() or
>>>>> nearby.
>>>>
>>>> It is in collect_mntinfo() but it seems calling lookup_ns_by_id() in
>>>> unix_process_name() does return an ns_id but ns.mnt->mntinfo_tree is not
>>>> initialized. Is mntinfo a global that is identical for all ns_ids in the linked
>>>> list? If not how should they be initialized?
>>>
>>> Scratch that, collect_mntinfo() is per namespace. The question is when to call
>>> it. One option is that as soon as you add a new ns_id you call collect_mntinfo()
>>> or you call it *first* when you need it, i.e. you check whether mntinfo_tree ==
>>> NULL and if so call collect_mntinfo() on the corresponding ns_id.
>>
>> Actually collect_mntinfo() is called for every namespace found already. Can you
>> trace back why the ns you see isn't yet collect_mntinfo()-ed at the time the
>> unix_process_name() is?
> 
> This mainly affects cr-check.c and there only collect_mntinfo() is called. I'd
> argue that when unix_process_name() is called and calls lookup_ns_by_id() a
> namespace is retrieved for which we did not yet collect_mntinfo() in cr-check.c.
> So one possibility is to use collect_mnt_namespaces() instead of
> collect_mntinfo().

If its only about cr-check, then of course, do whatever is needed, we don't fight
for performance in this action :)

-- Pavel