[Devel] [PATCH] ext4: release leaked posix acl in ext4_acl_chmod

[Stanislav Kinsburskiy]

Note: only rh7-3.10.0-693.17.1.el7-based kernels are affected.
I.e. starting from rh7-3.10.0-693.17.1.vz7.43.1.

Posix acl is used to convert of an extended attribute, provided by user to
ext4 attributes. In particular to i_mode in case of ACL_TYPE_ACCESS
request.
IOW, this object is allocated, used for convertion, not stored anywhere
and
must be freed.
However posix_acl_update_mode() can zerofy the pointer to support
ext4_set_acl() logic, but then the object is leaked.
So, fix it by releasing new temporary pointer with the same value instead
of
acl pointer.

In scope of https://jira.sw.ru/browse/PSBM-81384

RHEL bug URL: https://bugzilla.redhat.com/show_bug.cgi?id=1543020

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 fs/ext4/acl.c |6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
index f8a38a2..046b338 100644
--- a/fs/ext4/acl.c
+++ b/fs/ext4/acl.c
@@ -297,7 +297,7 @@ ext4_init_acl(handle_t *handle, struct inode *inode, struct 
inode *dir)
 int
 ext4_acl_chmod(struct inode *inode)
 {
-   struct posix_acl *acl;
+   struct posix_acl *acl, *real_acl;
handle_t *handle;
int retries = 0;
int error;
@@ -315,6 +315,8 @@ ext4_acl_chmod(struct inode *inode)
error = posix_acl_chmod(, GFP_KERNEL, inode->i_mode);
if (error)
return error;
+
+   real_acl = acl;
 retry:
handle = ext4_journal_start(inode, EXT4_HT_XATTR,
ext4_jbd2_credits_xattr(inode));
@@ -341,7 +343,7 @@ ext4_acl_chmod(struct inode *inode)
ext4_should_retry_alloc(inode->i_sb, ))
goto retry;
 out:
-   posix_acl_release(acl);
+   posix_acl_release(real_acl);
return error;
 }
 

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH v2] ext4: release leaked posix acl in ext4_xattr_set_acl

[Stanislav Kinsburskiy]

Note: only rh7-3.10.0-693.17.1.el7-based kernels are affcted.
I.e. starting from rh7-3.10.0-693.17.1.vz7.43.1.

Posix acl is used to convert of an extended attribute, provided by user to
ext4 attributes. In particular to i_mode in case of ACL_TYPE_ACCESS request.
IOW, this object is allocated, used for convertion, not stored anywhere and
must be freed.
However posix_acl_update_mode() can zerofy the pointer to support
ext4_set_acl() logic, but then the object is leaked.
So, fix it by releasing new temporary pointer with the same value instead of
acl pointer.

https://jira.sw.ru/browse/PSBM-81384

RHEL bug URL: https://bugzilla.redhat.com/show_bug.cgi?id=1543020

v2: Added affected kernel version + RHEL bug URL

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 fs/ext4/acl.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
index 917e819..f8a38a2 100644
--- a/fs/ext4/acl.c
+++ b/fs/ext4/acl.c
@@ -403,7 +403,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char *name, 
const void *value,
 {
struct inode *inode = dentry->d_inode;
handle_t *handle;
-   struct posix_acl *acl;
+   struct posix_acl *acl, *real_acl;
int error, retries = 0;
int update_mode = 0;
umode_t mode = inode->i_mode;
@@ -416,7 +416,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char *name, 
const void *value,
return -EPERM;
 
if (value) {
-   acl = posix_acl_from_xattr(_user_ns, value, size);
+   acl = real_acl = posix_acl_from_xattr(_user_ns, value, 
size);
if (IS_ERR(acl))
return PTR_ERR(acl);
else if (acl) {
@@ -425,7 +425,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char *name, 
const void *value,
goto release_and_out;
}
} else
-   acl = NULL;
+   acl = real_acl = NULL;
 
 retry:
handle = ext4_journal_start(inode, EXT4_HT_XATTR,
@@ -452,7 +452,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char *name, 
const void *value,
goto retry;
 
 release_and_out:
-   posix_acl_release(acl);
+   posix_acl_release(real_acl);
return error;
 }
 

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] ext4: release leaked posix acl in ext4_xattr_set_acl

[Stanislav Kinsburskiy]

07.02.2018 14:51, Dmitry Monakhov пишет:
> Stanislav Kinsburskiy <skinsbur...@virtuozzo.com> writes:
> 
>> Posix acl is used to convert of an extended attribute, provided by user to
>> ext4 attributes. In particular to I-mode in case of ACL_TYPE_ACCESS request.
>> IOW, this object is allocated, used for convertion, not stored anywhere and
>> must be freed.
>> However posix_acl_update_mode() can zerofy the pointer to support
>> ext4_set_acl() logic, but then the object is leaked.
>> So, fix it by releasing new temporary pointer with the same value instead of
>> acl pointer.
> So you are telling me that:
> ext4_xattr_set_acl
> L1 acl = posix_acl_from_xattr 
> L2 -> ext4_set_acl(handle, inode, type, acl)
> L3->posix_acl_update_mode(inode, >i_mode, )
>   *acl = NULL;
>   You are saying that instruction above can affect value at L1?
>   HOW? acl passed to ext4_set_acl() by value, so
>   posix_acl_update_mode() can affect value only in L2 and L3 but not L1. 
> 
> Stas, have you drunk a lousy beer today?

OMG, Dmitry... Looks like storage work clouded your mind and you are looking to 
some other sources.
There is no "L3" in the bug.
Because posix_acl_update_mode() in called from ext4_xattr_set_acl() (i.e on 
"L2").
Thus pointer is set to NULL on "L2" and released on "L1".
IOW, pointer is leaked.


>>
>> https://jira.sw.ru/browse/PSBM-81384
>>
>> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
>> ---
>>  fs/ext4/acl.c |8 
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
>> index 917e819..2640d7b 100644
>> --- a/fs/ext4/acl.c
>> +++ b/fs/ext4/acl.c
>> @@ -403,7 +403,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char 
>> *name, const void *value,
>>  {
>>  struct inode *inode = dentry->d_inode;
>>  handle_t *handle;
>> -struct posix_acl *acl;
>> +struct posix_acl *acl, *tmp;
>>  int error, retries = 0;
>>  int update_mode = 0;
>>  umode_t mode = inode->i_mode;
>> @@ -416,7 +416,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char 
>> *name, const void *value,
>>  return -EPERM;
>>  
>>  if (value) {
>> -acl = posix_acl_from_xattr(_user_ns, value, size);
>> +acl = tmp = posix_acl_from_xattr(_user_ns, value, size);
>>  if (IS_ERR(acl))
>>  return PTR_ERR(acl);
>>  else if (acl) {
>> @@ -425,7 +425,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char 
>> *name, const void *value,
>>  goto release_and_out;
>>  }
>>  } else
>> -acl = NULL;
>> +acl = tmp = NULL;
>>  
>>  retry:
>>  handle = ext4_journal_start(inode, EXT4_HT_XATTR,
>> @@ -452,7 +452,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char 
>> *name, const void *value,
>>  goto retry;
>>  
>>  release_and_out:
>> -posix_acl_release(acl);
>> +posix_acl_release(tmp);
>>  return error;
>>  }
>>  
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] ext4: release leaked posix acl in ext4_xattr_set_acl

[Stanislav Kinsburskiy]

Posix acl is used to convert of an extended attribute, provided by user to
ext4 attributes. In particular to I-mode in case of ACL_TYPE_ACCESS request.
IOW, this object is allocated, used for convertion, not stored anywhere and
must be freed.
However posix_acl_update_mode() can zerofy the pointer to support
ext4_set_acl() logic, but then the object is leaked.
So, fix it by releasing new temporary pointer with the same value instead of
acl pointer.

https://jira.sw.ru/browse/PSBM-81384

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 fs/ext4/acl.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
index 917e819..2640d7b 100644
--- a/fs/ext4/acl.c
+++ b/fs/ext4/acl.c
@@ -403,7 +403,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char *name, 
const void *value,
 {
struct inode *inode = dentry->d_inode;
handle_t *handle;
-   struct posix_acl *acl;
+   struct posix_acl *acl, *tmp;
int error, retries = 0;
int update_mode = 0;
umode_t mode = inode->i_mode;
@@ -416,7 +416,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char *name, 
const void *value,
return -EPERM;
 
if (value) {
-   acl = posix_acl_from_xattr(_user_ns, value, size);
+   acl = tmp = posix_acl_from_xattr(_user_ns, value, size);
if (IS_ERR(acl))
return PTR_ERR(acl);
else if (acl) {
@@ -425,7 +425,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char *name, 
const void *value,
goto release_and_out;
}
} else
-   acl = NULL;
+   acl = tmp = NULL;
 
 retry:
handle = ext4_journal_start(inode, EXT4_HT_XATTR,
@@ -452,7 +452,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char *name, 
const void *value,
goto retry;
 
 release_and_out:
-   posix_acl_release(acl);
+   posix_acl_release(tmp);
return error;
 }
 

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH v3 1/2] spfs: Handle non-zero exit status in cleanup_spfs_mount()

[Stanislav Kinsburskiy]

Looks good to me

Acked-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>


23.01.2018 11:43, Kirill Tkhai пишет:
> Consider non-zero exit status of spfs similar to killed
> status and do the same cleanups.
> 
> v3: New
> 
> Signed-off-by: Kirill Tkhai <ktk...@virtuozzo.com>
> ---
>  manager/context.c |8 
>  manager/spfs.c|8 
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/manager/context.c b/manager/context.c
> index 1eb37c9..73d5ada 100644
> --- a/manager/context.c
> +++ b/manager/context.c
> @@ -45,12 +45,12 @@ const char *mgr_ovz_id(void)
>  static void cleanup_spfs_mount(struct spfs_manager_context_s *ctx,
>  struct spfs_info_s *info, int status)
>  {
> - bool killed = WIFSIGNALED(status);
> + bool failed = WIFSIGNALED(status) || !!WEXITSTATUS(status);
>  
>   pr_debug("removing info %s from the list\n", info->mnt.id);
>  
> - if (killed)
> - /* SPFS master was killed. We need to release the reference */
> + if (failed)
> + /* SPFS master was failed. We need to release the reference */
>   spfs_release_mnt(info);
>  
>   info->dead = true;
> @@ -59,7 +59,7 @@ static void cleanup_spfs_mount(struct 
> spfs_manager_context_s *ctx,
>   if (unlink(info->socket_path))
>   pr_perror("failed to unlink %s", info->socket_path);
>  
> - spfs_cleanup_env(info, killed);
> + spfs_cleanup_env(info, failed);
>  
>   close_namespaces(info->ns_fds);
>  }
> diff --git a/manager/spfs.c b/manager/spfs.c
> index 3e0f667..99845b1 100644
> --- a/manager/spfs.c
> +++ b/manager/spfs.c
> @@ -409,9 +409,9 @@ int spfs_prepare_env(struct spfs_info_s *info, const char 
> *proxy_dir)
>   return err ? err : res;
>  }
>  
> -static int __spfs_cleanup_env(struct spfs_info_s *info, bool killed)
> +static int __spfs_cleanup_env(struct spfs_info_s *info, bool failed)
>  {
> - if (killed && umount(info->work_dir)) {
> + if (failed && umount(info->work_dir)) {
>   pr_perror("failed to umount %s", info->work_dir);
>   return -errno;
>   }
> @@ -423,7 +423,7 @@ static int __spfs_cleanup_env(struct spfs_info_s *info, 
> bool killed)
>   return 0;
>  }
>  
> -int spfs_cleanup_env(struct spfs_info_s *info, bool killed)
> +int spfs_cleanup_env(struct spfs_info_s *info, bool failed)
>  {
>   int err, res;
>   unsigned orig_ns_mask;
> @@ -432,7 +432,7 @@ int spfs_cleanup_env(struct spfs_info_s *info, bool 
> killed)
>   if (res)
>   return res;
>  
> - err = __spfs_cleanup_env(info, killed);
> + err = __spfs_cleanup_env(info, failed);
>  
>   res = leave_spfs_context(info, orig_ns_mask);
>  
> 
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] spfs: Main process wakes and kills its children on exit

[Stanislav Kinsburskiy]

23.01.2018 11:19, Kirill Tkhai пишет:
> On 23.01.2018 13:18, Stanislav Kinsburskiy wrote:
>>
>>
>> 23.01.2018 11:17, Kirill Tkhai пишет:
>>> On 23.01.2018 13:14, Stanislav Kinsburskiy wrote:
>>>>
>>>>
>>>> 23.01.2018 11:09, Kirill Tkhai пишет:
>>>>> On 23.01.2018 13:07, Stanislav Kinsburskiy wrote:
>>>>>>
>>>>>>
>>>>>> 23.01.2018 10:51, Kirill Tkhai пишет:
>>>>>>> On 23.01.2018 12:48, Stanislav Kinsburskiy wrote:
>>>>>>>> Please, see a couple of nits below
>>>>>>>>
>>>>>>>> 23.01.2018 10:41, Kirill Tkhai пишет:
>>>>>>>>> Stanislav Kinsburskiy says:
>>>>>>>>>
>>>>>>>>> "SPFS manager has a special "--exit-with-spfs" options, which is used 
>>>>>>>>> by CRIU.
>>>>>>>>>  The idea of the option is simple: force SPFS manager to exit, when 
>>>>>>>>> it has some
>>>>>>>>>  SPFS processes among its children (i.e. spfs was mounted at least 
>>>>>>>>> once),
>>>>>>>>>  but all these processes have exited for whatever reason (which 
>>>>>>>>> usually happens
>>>>>>>>>  when restore has failed and spfs mounts where unmounted).
>>>>>>>>>  Although it works in overall (main SPFS manager process exits), its 
>>>>>>>>> children
>>>>>>>>>  (responsible to SPFS replacement) may wait on FUTEX for "release" 
>>>>>>>>> command
>>>>>>>>>  for corresponding SPFS mount and thus never stop until they are 
>>>>>>>>> killed".
>>>>>>>>>
>>>>>>>>> 1 spfs-manager
>>>>>>>>> 2   \_ spfs
>>>>>>>>> 3   \_ spfs-manager
>>>>>>>>> 4   \_ spfs
>>>>>>>>> 5   \_ spfs-manager
>>>>>>>>>
>>>>>>>>> 2 and 3 are pair of a mount, and 4 and 5 are pair of another mount.
>>>>>>>>> The patch makes spfs-manager 1 kill 3 in case of 2 exited.
>>>>>>>>>
>>>>>>>>> https://jira.sw.ru/browse/PSBM-80055
>>>>>>>>>
>>>>>>>>> Signed-off-by: Kirill Tkhai <ktk...@virtuozzo.com>
>>>>>>>>> ---
>>>>>>>>>  manager/context.c |4 
>>>>>>>>>  manager/spfs.c|1 +
>>>>>>>>>  2 files changed, 5 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/manager/context.c b/manager/context.c
>>>>>>>>> index 1eb37c9..4464a23 100644
>>>>>>>>> --- a/manager/context.c
>>>>>>>>> +++ b/manager/context.c
>>>>>>>>> @@ -53,6 +53,9 @@ static void cleanup_spfs_mount(struct 
>>>>>>>>> spfs_manager_context_s *ctx,
>>>>>>>>>   /* SPFS master was killed. We need to release the 
>>>>>>>>> reference */
>>>>>>>>>   spfs_release_mnt(info);
>>>>>>>>>  
>>>>>>>>> + if (killed || WEXITSTATUS(status))
>>>>>>>>> + kill(info->replacer, SIGKILL);
>>>>>>>>> +
>>>>>>>>
>>>>>>>> There is "if (killed)" check above.
>>>>>>>> Could you please move this hunk there?
>>>>>>>
>>>>>>> There is logical OR (||) in the hunk. How should I move it to 
>>>>>>> unconditional check?
>>>>>>>
>>>>>>
>>>>>> Ah, ok. Then let the check be as it is.
>>>>>> Could you please add warning message for this kill with the process pid 
>>>>>> being killed and the reason why (spfs was killed of exited with error)?
>>>>>
>>>>> Maybe we should call spfs_release_mnt() in case of exit status != 0 too?
>>>>>
>>>>
>>>> Well, this makes sense.
>>>> If spfs exited with non-zero result (either it was killed or exited due to 
>>>> some erro

Re: [Devel] [PATCH] spfs: Main process wakes and kills its children on exit

[Stanislav Kinsburskiy]

23.01.2018 11:17, Kirill Tkhai пишет:
> On 23.01.2018 13:14, Stanislav Kinsburskiy wrote:
>>
>>
>> 23.01.2018 11:09, Kirill Tkhai пишет:
>>> On 23.01.2018 13:07, Stanislav Kinsburskiy wrote:
>>>>
>>>>
>>>> 23.01.2018 10:51, Kirill Tkhai пишет:
>>>>> On 23.01.2018 12:48, Stanislav Kinsburskiy wrote:
>>>>>> Please, see a couple of nits below
>>>>>>
>>>>>> 23.01.2018 10:41, Kirill Tkhai пишет:
>>>>>>> Stanislav Kinsburskiy says:
>>>>>>>
>>>>>>> "SPFS manager has a special "--exit-with-spfs" options, which is used 
>>>>>>> by CRIU.
>>>>>>>  The idea of the option is simple: force SPFS manager to exit, when it 
>>>>>>> has some
>>>>>>>  SPFS processes among its children (i.e. spfs was mounted at least 
>>>>>>> once),
>>>>>>>  but all these processes have exited for whatever reason (which usually 
>>>>>>> happens
>>>>>>>  when restore has failed and spfs mounts where unmounted).
>>>>>>>  Although it works in overall (main SPFS manager process exits), its 
>>>>>>> children
>>>>>>>  (responsible to SPFS replacement) may wait on FUTEX for "release" 
>>>>>>> command
>>>>>>>  for corresponding SPFS mount and thus never stop until they are 
>>>>>>> killed".
>>>>>>>
>>>>>>> 1 spfs-manager
>>>>>>> 2   \_ spfs
>>>>>>> 3   \_ spfs-manager
>>>>>>> 4   \_ spfs
>>>>>>> 5   \_ spfs-manager
>>>>>>>
>>>>>>> 2 and 3 are pair of a mount, and 4 and 5 are pair of another mount.
>>>>>>> The patch makes spfs-manager 1 kill 3 in case of 2 exited.
>>>>>>>
>>>>>>> https://jira.sw.ru/browse/PSBM-80055
>>>>>>>
>>>>>>> Signed-off-by: Kirill Tkhai <ktk...@virtuozzo.com>
>>>>>>> ---
>>>>>>>  manager/context.c |4 
>>>>>>>  manager/spfs.c|1 +
>>>>>>>  2 files changed, 5 insertions(+)
>>>>>>>
>>>>>>> diff --git a/manager/context.c b/manager/context.c
>>>>>>> index 1eb37c9..4464a23 100644
>>>>>>> --- a/manager/context.c
>>>>>>> +++ b/manager/context.c
>>>>>>> @@ -53,6 +53,9 @@ static void cleanup_spfs_mount(struct 
>>>>>>> spfs_manager_context_s *ctx,
>>>>>>> /* SPFS master was killed. We need to release the 
>>>>>>> reference */
>>>>>>> spfs_release_mnt(info);
>>>>>>>  
>>>>>>> +   if (killed || WEXITSTATUS(status))
>>>>>>> +   kill(info->replacer, SIGKILL);
>>>>>>> +
>>>>>>
>>>>>> There is "if (killed)" check above.
>>>>>> Could you please move this hunk there?
>>>>>
>>>>> There is logical OR (||) in the hunk. How should I move it to 
>>>>> unconditional check?
>>>>>
>>>>
>>>> Ah, ok. Then let the check be as it is.
>>>> Could you please add warning message for this kill with the process pid 
>>>> being killed and the reason why (spfs was killed of exited with error)?
>>>
>>> Maybe we should call spfs_release_mnt() in case of exit status != 0 too?
>>>
>>
>> Well, this makes sense.
>> If spfs exited with non-zero result (either it was killed or exited due to 
>> some error), then there is no need in replacer. And there is no need in the 
>> reference.
>> So, probably this check you proposed should be used for both 
>> spfs_release_mnt(info) and kill(info->replacer, SIGKILL).
> 
> Are you OK with this change sent in the only patch, or should we use one more 
> patch?
> 

I think one patch is OK. Only mention the change explicitly in the patch 
description, please.
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] spfs: Main process wakes and kills its children on exit

[Stanislav Kinsburskiy]

23.01.2018 11:09, Kirill Tkhai пишет:
> On 23.01.2018 13:07, Stanislav Kinsburskiy wrote:
>>
>>
>> 23.01.2018 10:51, Kirill Tkhai пишет:
>>> On 23.01.2018 12:48, Stanislav Kinsburskiy wrote:
>>>> Please, see a couple of nits below
>>>>
>>>> 23.01.2018 10:41, Kirill Tkhai пишет:
>>>>> Stanislav Kinsburskiy says:
>>>>>
>>>>> "SPFS manager has a special "--exit-with-spfs" options, which is used by 
>>>>> CRIU.
>>>>>  The idea of the option is simple: force SPFS manager to exit, when it 
>>>>> has some
>>>>>  SPFS processes among its children (i.e. spfs was mounted at least once),
>>>>>  but all these processes have exited for whatever reason (which usually 
>>>>> happens
>>>>>  when restore has failed and spfs mounts where unmounted).
>>>>>  Although it works in overall (main SPFS manager process exits), its 
>>>>> children
>>>>>  (responsible to SPFS replacement) may wait on FUTEX for "release" command
>>>>>  for corresponding SPFS mount and thus never stop until they are killed".
>>>>>
>>>>> 1 spfs-manager
>>>>> 2   \_ spfs
>>>>> 3   \_ spfs-manager
>>>>> 4   \_ spfs
>>>>> 5   \_ spfs-manager
>>>>>
>>>>> 2 and 3 are pair of a mount, and 4 and 5 are pair of another mount.
>>>>> The patch makes spfs-manager 1 kill 3 in case of 2 exited.
>>>>>
>>>>> https://jira.sw.ru/browse/PSBM-80055
>>>>>
>>>>> Signed-off-by: Kirill Tkhai <ktk...@virtuozzo.com>
>>>>> ---
>>>>>  manager/context.c |4 
>>>>>  manager/spfs.c|1 +
>>>>>  2 files changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/manager/context.c b/manager/context.c
>>>>> index 1eb37c9..4464a23 100644
>>>>> --- a/manager/context.c
>>>>> +++ b/manager/context.c
>>>>> @@ -53,6 +53,9 @@ static void cleanup_spfs_mount(struct 
>>>>> spfs_manager_context_s *ctx,
>>>>>   /* SPFS master was killed. We need to release the reference */
>>>>>   spfs_release_mnt(info);
>>>>>  
>>>>> + if (killed || WEXITSTATUS(status))
>>>>> + kill(info->replacer, SIGKILL);
>>>>> +
>>>>
>>>> There is "if (killed)" check above.
>>>> Could you please move this hunk there?
>>>
>>> There is logical OR (||) in the hunk. How should I move it to unconditional 
>>> check?
>>>
>>
>> Ah, ok. Then let the check be as it is.
>> Could you please add warning message for this kill with the process pid 
>> being killed and the reason why (spfs was killed of exited with error)?
> 
> Maybe we should call spfs_release_mnt() in case of exit status != 0 too?
> 

Well, this makes sense.
If spfs exited with non-zero result (either it was killed or exited due to some 
error), then there is no need in replacer. And there is no need in the 
reference.
So, probably this check you proposed should be used for both 
spfs_release_mnt(info) and kill(info->replacer, SIGKILL).



___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] spfs: Main process wakes and kills its children on exit

[Stanislav Kinsburskiy]

23.01.2018 10:51, Kirill Tkhai пишет:
> On 23.01.2018 12:48, Stanislav Kinsburskiy wrote:
>> Please, see a couple of nits below
>>
>> 23.01.2018 10:41, Kirill Tkhai пишет:
>>> Stanislav Kinsburskiy says:
>>>
>>> "SPFS manager has a special "--exit-with-spfs" options, which is used by 
>>> CRIU.
>>>  The idea of the option is simple: force SPFS manager to exit, when it has 
>>> some
>>>  SPFS processes among its children (i.e. spfs was mounted at least once),
>>>  but all these processes have exited for whatever reason (which usually 
>>> happens
>>>  when restore has failed and spfs mounts where unmounted).
>>>  Although it works in overall (main SPFS manager process exits), its 
>>> children
>>>  (responsible to SPFS replacement) may wait on FUTEX for "release" command
>>>  for corresponding SPFS mount and thus never stop until they are killed".
>>>
>>> 1 spfs-manager
>>> 2   \_ spfs
>>> 3   \_ spfs-manager
>>> 4   \_ spfs
>>> 5   \_ spfs-manager
>>>
>>> 2 and 3 are pair of a mount, and 4 and 5 are pair of another mount.
>>> The patch makes spfs-manager 1 kill 3 in case of 2 exited.
>>>
>>> https://jira.sw.ru/browse/PSBM-80055
>>>
>>> Signed-off-by: Kirill Tkhai <ktk...@virtuozzo.com>
>>> ---
>>>  manager/context.c |4 
>>>  manager/spfs.c|1 +
>>>  2 files changed, 5 insertions(+)
>>>
>>> diff --git a/manager/context.c b/manager/context.c
>>> index 1eb37c9..4464a23 100644
>>> --- a/manager/context.c
>>> +++ b/manager/context.c
>>> @@ -53,6 +53,9 @@ static void cleanup_spfs_mount(struct 
>>> spfs_manager_context_s *ctx,
>>> /* SPFS master was killed. We need to release the reference */
>>> spfs_release_mnt(info);
>>>  
>>> +   if (killed || WEXITSTATUS(status))
>>> +   kill(info->replacer, SIGKILL);
>>> +
>>
>> There is "if (killed)" check above.
>> Could you please move this hunk there?
> 
> There is logical OR (||) in the hunk. How should I move it to unconditional 
> check?
> 

Ah, ok. Then let the check be as it is.
Could you please add warning message for this kill with the process pid being 
killed and the reason why (spfs was killed of exited with error)?
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] spfs: Main process wakes and kills its children on exit

[Stanislav Kinsburskiy]

Please, see a couple of nits below

23.01.2018 10:41, Kirill Tkhai пишет:
> Stanislav Kinsburskiy says:
> 
> "SPFS manager has a special "--exit-with-spfs" options, which is used by CRIU.
>  The idea of the option is simple: force SPFS manager to exit, when it has 
> some
>  SPFS processes among its children (i.e. spfs was mounted at least once),
>  but all these processes have exited for whatever reason (which usually 
> happens
>  when restore has failed and spfs mounts where unmounted).
>  Although it works in overall (main SPFS manager process exits), its children
>  (responsible to SPFS replacement) may wait on FUTEX for "release" command
>  for corresponding SPFS mount and thus never stop until they are killed".
> 
> 1 spfs-manager
> 2   \_ spfs
> 3   \_ spfs-manager
> 4   \_ spfs
> 5   \_ spfs-manager
> 
> 2 and 3 are pair of a mount, and 4 and 5 are pair of another mount.
> The patch makes spfs-manager 1 kill 3 in case of 2 exited.
> 
> https://jira.sw.ru/browse/PSBM-80055
> 
> Signed-off-by: Kirill Tkhai <ktk...@virtuozzo.com>
> ---
>  manager/context.c |4 
>  manager/spfs.c|1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/manager/context.c b/manager/context.c
> index 1eb37c9..4464a23 100644
> --- a/manager/context.c
> +++ b/manager/context.c
> @@ -53,6 +53,9 @@ static void cleanup_spfs_mount(struct 
> spfs_manager_context_s *ctx,
>   /* SPFS master was killed. We need to release the reference */
>   spfs_release_mnt(info);
>  
> + if (killed || WEXITSTATUS(status))
> + kill(info->replacer, SIGKILL);
> +

There is "if (killed)" check above.
Could you please move this hunk there?

>   info->dead = true;
>   del_spfs_info(ctx->spfs_mounts, info);
>  
> @@ -88,6 +91,7 @@ static void sigchld_handler(int signal, siginfo_t *siginfo, 
> void *data)
>* corresponding fd.
>*/
>   spfs_release_mnt(info);
> + info->replacer = -1;
>   } else {
>   info = find_spfs_by_pid(ctx->spfs_mounts, pid);
>   if (info) {
> diff --git a/manager/spfs.c b/manager/spfs.c
> index 3e0f667..0b94587 100644
> --- a/manager/spfs.c
> +++ b/manager/spfs.c
> @@ -39,6 +39,7 @@ int create_spfs_info(const char *id,
>   pr_err("failed to allocate\n");
>   return -ENOMEM;
>   }
> + info->replacer = -1;
>  

Could you, please, move it down to the end of the function where unconditional 
actions take place?

>   err = init_mount_info(>mnt, id, mountpoint, ns_mountpoint);
>   if (err)
> 
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH 1/3] libvzctl-4.14: clone CT in new cgroup namespace

[Stanislav Kinsburskiy]

We are going to use it along with all the other namespaces.

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 lib/env_nsops.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/env_nsops.c b/lib/env_nsops.c
index 13e65b0..0771eb2 100644
--- a/lib/env_nsops.c
+++ b/lib/env_nsops.c
@@ -807,8 +807,10 @@ static int do_env_create(struct vzctl_env_handle *h, 
struct start_param *param)
}
param->init_p = init_p;
 
+#define CLONE_NEWCGROUP 0x0200  /* New cgroup namespace */
+
clone_flags |= CLONE_NEWUTS|CLONE_NEWPID|CLONE_NEWIPC|
-   CLONE_NEWNET|CLONE_NEWNS|CLONE_NEWUSER;
+   CLONE_NEWNET|CLONE_NEWNS|CLONE_NEWUSER|CLONE_NEWCGROUP;
pid = clone(real_ns_env_create,
child_stack + sizeof(child_stack),
clone_flags|SIGCHLD , (void *) param);

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH 2/3] libvzctl-4.14: join namespaces in explicitly provided order

[Stanislav Kinsburskiy]

This is needed to make sure, that mnt ns is the last (otherwise join other
namespaces after mnt ns will fail).

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 lib/env_nsops.c |   39 +++
 1 file changed, 11 insertions(+), 28 deletions(-)

diff --git a/lib/env_nsops.c b/lib/env_nsops.c
index 0771eb2..d885d1c 100644
--- a/lib/env_nsops.c
+++ b/lib/env_nsops.c
@@ -885,7 +885,7 @@ static int ns_is_env_run(struct vzctl_env_handle *h)
return cg_env_get_ve_state(EID(h));
 }
 
-int set_ns(pid_t pid, const char *name, int flags)
+static int set_ns(pid_t pid, const char *name, int flags)
 {
int ret, fd;
char path[PATH_MAX];
@@ -924,11 +924,10 @@ int enter_net_ns(struct vzctl_env_handle *h, pid_t 
*ct_pid)
 
 static int ns_env_enter(struct vzctl_env_handle *h, int flags)
 {
-   DIR *dp;
-   struct dirent *ep;
pid_t pid;
-   char path[PATH_MAX];
-   int ret;
+   int ret, i;
+   const char *ns[] = {"cgroup", "ipc", "net", "uts", "pid",
+   "pid_for_children", "user", "mnt"};
 
ret = reset_loginuid();
if (ret)
@@ -939,37 +938,21 @@ static int ns_env_enter(struct vzctl_env_handle *h, int 
flags)
 
logger(10, 0, "* Attach by pid %d", pid);
 
-   snprintf(path, sizeof(path), "/proc/%d/ns", pid);
-   dp = opendir(path);
-   if (dp == NULL)
-   return vzctl_err(-1, errno, "Unable to open dir %s", path);
-
ret = cg_attach_task(EID(h), getpid(), NULL, NULL);
if (ret)
-   goto err;
-
-   while ((ep = readdir (dp))) {
-   if (!strcmp(ep->d_name, ".") ||
-   !strcmp(ep->d_name, ".."))
-   continue;
+   return ret;
 
-   ret = set_ns(pid, ep->d_name, 0);
+   for (i = 0; i < sizeof(ns) / sizeof(ns[0]); ++i) {
+   ret = set_ns(pid, ns[i], 0);
if (ret)
-   goto err;
+   return ret;
}
 
/* Clear supplementary group IDs */
-   if (setgroups(0, NULL)) {
-   ret = vzctl_err(-1, errno, "ns_env_enter: setgroups()");
-   goto err;
-   }
-   
-   ret = set_personality32();
+   if (setgroups(0, NULL))
+   return vzctl_err(-1, errno, "ns_env_enter: setgroups()");
 
-err:
-   closedir(dp);
-
-   return ret;
+   return set_personality32();
 }
 
 static int ns_env_exec(struct vzctl_env_handle *h, struct exec_param *param,

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH 3/3] libvzctl-4.14: do not mount sysfs in container

[Stanislav Kinsburskiy]

Our current approach is to use one sysfs mount for all the containers, but set
limited visibility to sysfs dentries in a container.

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 lib/env.c |3 ---
 1 file changed, 3 deletions(-)

diff --git a/lib/env.c b/lib/env.c
index 1b03ce4..a284ec9 100644
--- a/lib/env.c
+++ b/lib/env.c
@@ -772,9 +772,6 @@ int pre_setup_env(const struct start_param *param)
if (setup_devtmpfs())
return VZCTL_E_SYSTEM;
 
-   if (stat_file("/sys"))
-   mount("sysfs", "/sys", "sysfs", 0, 0);
-
if (env->features->mask & VE_FEATURE_NFSD) {
mount("nfsd", "/proc/fs/nfsd", "nfsd", 0, 0);
make_dir("/var/lib/nfs/rpc_pipefs", 1);

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] 4.14: remove beancounter cgroup usage

[Stanislav Kinsburskiy]

This might be a temporary patch (since we don't have beaconters in 4.14 yet),
but it's needed to proceed with testing.

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 lib/cgroup.c|   30 +-
 lib/cgroup.h|1 -
 lib/env_nsops.c |   38 ++
 3 files changed, 3 insertions(+), 66 deletions(-)

diff --git a/lib/cgroup.c b/lib/cgroup.c
index cd71064..52dc013 100644
--- a/lib/cgroup.c
+++ b/lib/cgroup.c
@@ -56,7 +56,6 @@ static struct cg_ctl cg_ctl_map[] = {
{CG_DEVICES},
{CG_BLKIO},
{CG_FREEZER},
-   {CG_UB, 1},
{CG_VE, 1},
{CG_PERF_EVENT},
{CG_HUGETLB},
@@ -741,39 +740,12 @@ int cg_env_set_memory(const char *ctid, const char *name, 
unsigned long value)
 
 int cg_env_set_ub(const char *ctid, const char *name, unsigned long b, 
unsigned long l)
 {
-   int rc;
-   char _name[STR_SIZE];
-
-   snprintf(_name, sizeof(_name), "beancounter.%s.barrier", name);
-   rc = cg_set_ul(ctid, CG_UB, _name, b);
-   if (rc)
-   return rc;
-
-   snprintf(_name, sizeof(_name), "beancounter.%s.limit", name);
-   return cg_set_ul(ctid, CG_UB, _name, l);
+   return 0;
 }
 
 static int cg_env_set_io(const char *ctid, const char *name, unsigned int 
speed,
unsigned int burst, unsigned int latency)
 {
-   int ret;
-   char buf[STR_SIZE];
-
-   snprintf(buf, sizeof(buf), CG_UB".%s.speed", name);
-   ret = cg_set_ul(ctid, CG_UB, buf, speed);
-   if (ret)
-   return ret;
-
-   snprintf(buf, sizeof(buf), CG_UB".%s.burst", name);
-   ret = cg_set_ul(ctid, CG_UB, buf, burst);
-   if (ret)
-   return ret;
-
-   snprintf(buf, sizeof(buf), CG_UB".%s.latency", name);
-   ret = cg_set_ul(ctid, CG_UB, buf, latency);
-   if (ret)
-   return ret;
-
return 0;
 }
 
diff --git a/lib/cgroup.h b/lib/cgroup.h
index 53429e9..c0b308d 100644
--- a/lib/cgroup.h
+++ b/lib/cgroup.h
@@ -29,7 +29,6 @@
 #define CG_MEMORY  "memory"
 #define CG_NET_CLS "net_cls"
 #define CG_VE  "ve"
-#define CG_UB  "beancounter"
 #define CG_BLKIO   "blkio"
 #define CG_FREEZER "freezer"
 #define CG_PERF_EVENT  "perf_event"
diff --git a/lib/env_nsops.c b/lib/env_nsops.c
index a30eb6a..13e65b0 100644
--- a/lib/env_nsops.c
+++ b/lib/env_nsops.c
@@ -566,24 +566,12 @@ static int init_env_cgroup(struct vzctl_env_handle *h, 
int flags)
"cpuset.cpus",
"cpuset.mems"
};
-   char *bc[] = {
-   "beancounter.memory",
-   "beancounter.blkio"
-   };
 
logger(10, 0, "* init Container cgroup");
if (h->veid && cg_set_veid(EID(h), h->veid) == -1)
return vzctl_err(VZCTL_E_RESOURCE, 0,
"Failed to set VEID=%u", h->veid);
 
-   /* Bind beancounter with blkio/memory cgroups */
-   for (i = 0; i < sizeof(bc)/sizeof(bc[0]); i++) {
-   snprintf(buf, sizeof(buf), "/%s/%s", cg_get_slice_name(), 
EID(h));
-   ret = cg_set_param(EID(h), CG_UB, bc[i], buf);
-   if (ret)
-   return ret;
-   }
-
ret = cg_env_set_memory(h->ctid, "memory.use_hierarchy", 1);
if (ret)
return ret;
@@ -1674,14 +1662,7 @@ static int ns_set_iolimit(struct vzctl_env_handle *h, 
unsigned int speed)
 
 static int ns_get_iolimit(struct vzctl_env_handle *h, unsigned int *speed)
 {
-   int ret;
-   unsigned long n;
-
-   ret = cg_get_ul(EID(h), CG_UB, CG_UB".iolimit.speed", );
-
-   *speed = (unsigned int) n;
-
-   return ret;
+   return 0;
 }
 
 static int ns_set_ioprio(struct vzctl_env_handle *h, int prio)
@@ -1712,13 +1693,7 @@ static int ns_set_iopslimit(struct vzctl_env_handle *h, 
unsigned int speed)
 
 static int ns_get_iopslimit(struct vzctl_env_handle *h, unsigned int *speed)
 {
-   int ret;
-   unsigned long n;
-
-   ret = cg_get_ul(EID(h), CG_UB, CG_UB".iopslimit.speed", );
-   *speed = (unsigned int) n;
-
-   return ret;
+   return 0;
 }
 
 static int ns_get_runtime_param(struct vzctl_env_handle *h, int flags)
@@ -1761,11 +1736,6 @@ int vzctl2_set_limits(struct vzctl_env_handle *h, int 
release)
 {
int ret;
 
-   if (release) {
-   cg_set_ul("", CG_UB, "tasks", getpid());
-   return cg_destroy_cgroup(EID(h));
-   }
-
if (EMPTY_CTID(h->ctid))
vzctl2_generate_ctid(EID(h));
 
@@ -1786,10 +1756,6 @@ int vzctl2_set_limits(struct vzctl_env_handle *h, int 
release)
if (ret)
 

[Devel] [PATCH] criu: return original error code from bc_failcnt_check() if failed

[Stanislav Kinsburskiy]

Currently this helper doesn't see "/proc/bc/%s/resources" (another mount ns?).
But this leads to a situation, when error is swallowed by the helper.
I.e. it returns 0 even if there was an error during restore.
So, let's return original error code instead.

https://jira.sw.ru/browse/PSBM-80056

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 criu/cr-restore.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/criu/cr-restore.c b/criu/cr-restore.c
index bd36523..6337c0f 100644
--- a/criu/cr-restore.c
+++ b/criu/cr-restore.c
@@ -2034,7 +2034,7 @@ static int bc_failcnt_check(int ret)
snprintf(buf, sizeof(buf), "/proc/bc/%s/resources", veid);
f = fopen(buf, "r");
if (!f) {
-   return 0;
+   return ret;
}

while (fgets(buf, sizeof(buf), f)) {

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] spfs: start SPFS manager in containers network namespace

[Stanislav Kinsburskiy]

This is needed because in case of killing of a container with SPFS manager
inside (due to any restore error) all the network namespaces of container
processes will be marked to drop SUNRPC packets (libvzctl does it on fast stop).

This in turn happens, because we want to be able to kill container with
blocked network and NFS mount inside.

Thus all the processes, belonging to VE cgroup have to have containers
network namespaces otherwise SUNRPC trafic is dropped in init network
namespace, like it happens now.

https://jira.sw.ru/browse/PSBM-79733

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 criu/spfs.c |   21 -
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/criu/spfs.c b/criu/spfs.c
index 6ce2ac8..d46fe11 100644
--- a/criu/spfs.c
+++ b/criu/spfs.c
@@ -21,6 +21,7 @@
 #include "spfs.h"
 #include "proc_parse.h"
 #include "cgroup.h"
+#include "net.h"
 
 #define SPFS_MANAGER_WORK_DIR  "/run/spfs-manager/%d"
 #define VE_SPFS_MANAGER_WORK_DIR   "/vz/private/%s/dump/spfs-manager/%d"
@@ -121,7 +122,7 @@ static char *spfs_manager_log_dir(void)
return work_dir;
 }
 
-static int start_spfs_manager(void)
+static int __start_spfs_manager(void)
 {
char *spfs_manager = "spfs-manager";
char *socket_path = spfs_manager_socket_path();
@@ -159,6 +160,24 @@ static int start_spfs_manager(void)
return sock;
 }
 
+static int start_spfs_manager(void)
+{
+   int old_net_ns, sock;
+
+   if (switch_ns(root_item->pid->real, _ns_desc, _net_ns)) {
+   pr_err("failed to switch to containers network namespace\n");
+   return -1;
+   }
+
+   sock = __start_spfs_manager();
+
+   if (restore_ns(old_net_ns, _ns_desc)) {
+   pr_err("failed to restore original usernsd network 
namespace\n");
+   return -1;
+   }
+   return sock;
+}
+
 static int get_spfs_mngr_sock(void *start, int fd, pid_t pid)
 {
int sock;

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH v2 6/7] spfs: return duplicated socket from usernsd

[Stanislav Kinsburskiy]

Usernsd closes socket when sent.

https://jira.sw.ru/browse/PSBM-79462

v2: duplicate only service descriptor

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 criu/spfs.c |7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/criu/spfs.c b/criu/spfs.c
index 7ce30e7..6ce2ac8 100644
--- a/criu/spfs.c
+++ b/criu/spfs.c
@@ -164,8 +164,13 @@ static int get_spfs_mngr_sock(void *start, int fd, pid_t 
pid)
int sock;
 
sock = get_service_fd(SPFS_MNGR_SK);
-   if (sock < 0 && start)
+   if (sock != -1) {
+   sock = dup(sock);
+   if (sock < 0)
+   pr_perror("failed to duplicate SPFS manager socket\n");
+   } else if (start)
sock = start_spfs_manager();
+
return sock;
 }
 

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH v2 4/7] spfs: improve SPFS manager start debug and error output

[Stanislav Kinsburskiy]

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 criu/spfs.c |   12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/criu/spfs.c b/criu/spfs.c
index 223bf93..89fb9f9 100644
--- a/criu/spfs.c
+++ b/criu/spfs.c
@@ -127,6 +127,8 @@ static int start_spfs_manager(void)
char *socket_path = spfs_manager_socket_path();
int err = -ENOMEM, sock;
 
+   pr_info("Starting SPFS manager\n");
+
err = cr_system(-1, -1, -1, spfs_manager,
(char *[]){ "spfs-manager", "-",
 "-d",
@@ -135,13 +137,17 @@ static int start_spfs_manager(void)
 "--log-dir", spfs_manager_log_dir(),
 "--exit-with-spfs", NULL },
0);
-   pr_info("%s: spfs manager start result: %d\n", __func__, err);
-   if (err)
+   if (err) {
+   pr_err("failed to start SPFS manager binary: %d\n", err);
return err;
+   }
 
sock = sock_seqpacket_connect(socket_path);
-   if (sock < 0)
+   if (sock < 0) {
+   pr_err("failed to connect to SPFS manager via %s: %d\n",
+   socket_path, err);
return sock;
+   }
 
err = install_service_fd(SPFS_MNGR_SK, sock);
if (err < 0) {

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH v2 2/7] spfs: improve error and debug output for spfs_mount()

[Stanislav Kinsburskiy]

Use request_spfs_mngr_sock() for both start and socket request.

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 criu/spfs.c |   15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/criu/spfs.c b/criu/spfs.c
index 0d44b72..1ead01b 100644
--- a/criu/spfs.c
+++ b/criu/spfs.c
@@ -292,18 +292,23 @@ int spfs_mount(struct mount_info *mi, const char *source,
 
sock = start_spfs_mngr();
if (sock < 0) {
-   pr_err("failed to mount NFS to path %s\n", mi->mountpoint);
-   return sock;
+   pr_err("failed to connect to SPFS manager: %d\n", sock);
+   ret = sock;
+   goto err;
}
-
ret = spfs_request_mount(sock, mi, source, filesystemtype, mountflags);
close(sock);
if (ret) {
-   pr_err("mount of %s (%s) failed: %d\n", source, filesystemtype, 
ret);
-   return ret;
+   pr_err("mount request for %s (%s) failed: %d\n",
+   source, filesystemtype, ret);
+   goto err;
}
 
return 0;
+
+err:
+   pr_err("failed to mount NFS to path %s\n", mi->mountpoint);
+   return ret;
 }
 
 int spfs_set_env(void)

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH v2 3/7] spfs: remove redundant spfs_service_fd() helper

[Stanislav Kinsburskiy]

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 criu/spfs.c |9 +
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/criu/spfs.c b/criu/spfs.c
index 1ead01b..223bf93 100644
--- a/criu/spfs.c
+++ b/criu/spfs.c
@@ -336,16 +336,9 @@ int spfs_set_env(void)
return 0;
 }
 
-static int spfs_service_fd(void *arg, int fd, pid_t pid)
-{
-   return get_service_fd(SPFS_MNGR_SK);
-}
-
 static int spfs_service_is_active(void *arg, int fd, pid_t pid)
 {
-   if (spfs_service_fd(arg, fd, pid) < 0)
-   return 0;
-   return 1;
+   return get_service_fd(SPFS_MNGR_SK) < 0 ? 0 : 1;
 }
 
 int spfs_mngr_status(bool *active)

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH v2 1/7] spfs: introduce request_spfs_mngr_sock)() helper

[Stanislav Kinsburskiy]

This generic helper will be responsible for both SPFS manager start via
usernsd and socket request.

v2:
Do not use variable, but explicitly provide fake void pointer ad boolean value
to request_spfs_mngr_sock()

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 criu/spfs.c |   43 ---
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/criu/spfs.c b/criu/spfs.c
index a5f6031..0d44b72 100644
--- a/criu/spfs.c
+++ b/criu/spfs.c
@@ -153,16 +153,36 @@ static int start_spfs_manager(void)
return sock;
 }
 
-static int get_spfs_mngr_sock(void *arg, int fd, pid_t pid)
+static int get_spfs_mngr_sock(void *start, int fd, pid_t pid)
 {
int sock;
 
sock = get_service_fd(SPFS_MNGR_SK);
-   if (sock < 0)
+   if (sock < 0 && start)
sock = start_spfs_manager();
return sock;
 }
 
+static int request_spfs_mngr_sock(bool *start_mngr)
+{
+   int ns_fd;
+   int sock;
+
+   ns_fd = open_proc(PROC_SELF, "ns");
+   if (ns_fd < 0)
+   return ns_fd;
+
+   sock = userns_call(get_spfs_mngr_sock, UNS_FDOUT, start_mngr, 0, ns_fd);
+
+   close(ns_fd);
+   return sock;
+}
+
+static int start_spfs_mngr(void)
+{
+   return request_spfs_mngr_sock((void *)1);
+}
+
 static int spfs_request_mount(int sock, struct mount_info *mi, const char 
*source,
  const char *type, unsigned long mountflags)
 {
@@ -268,15 +288,9 @@ int spfs_mount(struct mount_info *mi, const char *source,
   const char *filesystemtype, unsigned long mountflags)
 {
int ret;
-   int ns_fd;
int sock;
 
-   ns_fd = open_proc(PROC_SELF, "ns");
-   if (ns_fd < 0)
-   return ns_fd;
-
-   sock = userns_call(get_spfs_mngr_sock, UNS_FDOUT, NULL, 0, ns_fd);
-   close(ns_fd);
+   sock = start_spfs_mngr();
if (sock < 0) {
pr_err("failed to mount NFS to path %s\n", mi->mountpoint);
return sock;
@@ -345,16 +359,7 @@ int spfs_mngr_status(bool *active)
 
 int spfs_mngr_sock(void)
 {
-   int ns_fd, fd;
-
-   ns_fd = open_proc(PROC_SELF, "ns");
-   if (ns_fd < 0)
-   return ns_fd;
-
-   fd = userns_call(spfs_service_fd, UNS_FDOUT, NULL, 0, ns_fd);
-
-   close(ns_fd);
-   return fd;
+   return request_spfs_mngr_sock((void *)0);
 }
 
 int spfs_set_mode(int sock, const char *mode)

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH v2 0/7] spfs: duplicate socket before sending it from usernsd

[Stanislav Kinsburskiy]

Usernds closes socket once it was sent.
So, id should be duplicated before sending, if the socket is expected to be
send multiple times.

https://jira.sw.ru/browse/PSBM-79462

The following series implements...

---

Stanislav Kinsburskiy (7):
  spfs: introduce request_spfs_mngr_sock)() helper
  spfs: improve error and debug output for spfs_mount()
  spfs: remove redundant spfs_service_fd() helper
  spfs: improve SPFS manager start debug and error output
  spfs: improve prints in spfs_set_mode() and spfs_release_replace()
  spfs: return duplicated socket from usernsd
  spfs: switch mounts mode to STUB after root yard depopulation


 criu/cr-restore.c |   12 +++
 criu/spfs.c   |   92 +++--
 2 files changed, 59 insertions(+), 45 deletions(-)
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH 7/7] spfs: switch mountsmode to STUB after root yard depopulation

[Stanislav Kinsburskiy]

Otherwise CRIU stuck on SPFS mounts

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 criu/cr-restore.c |   12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/criu/cr-restore.c b/criu/cr-restore.c
index eb9f50d..bd36523 100644
--- a/criu/cr-restore.c
+++ b/criu/cr-restore.c
@@ -2253,12 +2253,6 @@ static int restore_root_task(struct pstree_item *init)
if (ret < 0)
goto out_kill;
 
-   if (spfs_is_running) {
-   ret = spfs_set_mode(spfs_sock, SPFS_MODE_STUB);
-   if (ret < 0)
-   goto out_kill;
-   }
-
if (fault_injected(FI_POST_RESTORE))
goto out_kill;
 
@@ -2279,6 +2273,12 @@ static int restore_root_task(struct pstree_item *init)
 
close_safe(_ns_fd);
 
+   if (spfs_is_running) {
+   ret = spfs_set_mode(spfs_sock, SPFS_MODE_STUB);
+   if (ret < 0)
+   goto out_kill;
+   }
+
if (write_restored_pid())
goto out_kill;
 

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH 6/7] spfs: return duplicated socket from usernsd

[Stanislav Kinsburskiy]

Usernsd closes socket when sent.

https://jira.sw.ru/browse/PSBM-79462

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 criu/spfs.c |   10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/criu/spfs.c b/criu/spfs.c
index da19179..857a167 100644
--- a/criu/spfs.c
+++ b/criu/spfs.c
@@ -162,11 +162,19 @@ static int start_spfs_manager(void)
 static int get_spfs_mngr_sock(void *start, int fd, pid_t pid)
 {
int sock;
+   int sfd;
 
sock = get_service_fd(SPFS_MNGR_SK);
if (sock < 0 && start)
sock = start_spfs_manager();
-   return sock;
+   if (sock < 0)
+   return sock;
+
+   sfd = dup(sock);
+   if (sfd < 0)
+   pr_perror("failed to duplicate socket %d", sock);
+
+   return sfd;
 }
 
 static int request_spfs_mngr_sock(bool *start_mngr)

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH 3/7] spfs: remove redundant spfs_service_fd() helper

[Stanislav Kinsburskiy]

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 criu/spfs.c |9 +
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/criu/spfs.c b/criu/spfs.c
index c202b14..be3bca4 100644
--- a/criu/spfs.c
+++ b/criu/spfs.c
@@ -338,16 +338,9 @@ int spfs_set_env(void)
return 0;
 }
 
-static int spfs_service_fd(void *arg, int fd, pid_t pid)
-{
-   return get_service_fd(SPFS_MNGR_SK);
-}
-
 static int spfs_service_is_active(void *arg, int fd, pid_t pid)
 {
-   if (spfs_service_fd(arg, fd, pid) < 0)
-   return 0;
-   return 1;
+   return get_service_fd(SPFS_MNGR_SK) < 0 ? 0 : 1;
 }
 
 int spfs_mngr_status(bool *active)

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH 1/7] spfs: introduce request_spfs_mngr_sock)() helper

[Stanislav Kinsburskiy]

This generic helper will be responsible for both SPFS manager start via
usernsd and socket request.

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 criu/spfs.c |   45 ++---
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/criu/spfs.c b/criu/spfs.c
index a5f6031..fff7b9f 100644
--- a/criu/spfs.c
+++ b/criu/spfs.c
@@ -153,16 +153,38 @@ static int start_spfs_manager(void)
return sock;
 }
 
-static int get_spfs_mngr_sock(void *arg, int fd, pid_t pid)
+static int get_spfs_mngr_sock(void *start, int fd, pid_t pid)
 {
int sock;
 
sock = get_service_fd(SPFS_MNGR_SK);
-   if (sock < 0)
+   if (sock < 0 && start)
sock = start_spfs_manager();
return sock;
 }
 
+static int request_spfs_mngr_sock(bool *start_mngr)
+{
+   int ns_fd;
+   int sock;
+
+   ns_fd = open_proc(PROC_SELF, "ns");
+   if (ns_fd < 0)
+   return ns_fd;
+
+   sock = userns_call(get_spfs_mngr_sock, UNS_FDOUT, start_mngr, 0, ns_fd);
+
+   close(ns_fd);
+   return sock;
+}
+
+static int start_spfs_mngr(void)
+{
+   bool start;
+
+   return request_spfs_mngr_sock();
+}
+
 static int spfs_request_mount(int sock, struct mount_info *mi, const char 
*source,
  const char *type, unsigned long mountflags)
 {
@@ -268,15 +290,9 @@ int spfs_mount(struct mount_info *mi, const char *source,
   const char *filesystemtype, unsigned long mountflags)
 {
int ret;
-   int ns_fd;
int sock;
 
-   ns_fd = open_proc(PROC_SELF, "ns");
-   if (ns_fd < 0)
-   return ns_fd;
-
-   sock = userns_call(get_spfs_mngr_sock, UNS_FDOUT, NULL, 0, ns_fd);
-   close(ns_fd);
+   sock = start_spfs_mngr();
if (sock < 0) {
pr_err("failed to mount NFS to path %s\n", mi->mountpoint);
return sock;
@@ -345,16 +361,7 @@ int spfs_mngr_status(bool *active)
 
 int spfs_mngr_sock(void)
 {
-   int ns_fd, fd;
-
-   ns_fd = open_proc(PROC_SELF, "ns");
-   if (ns_fd < 0)
-   return ns_fd;
-
-   fd = userns_call(spfs_service_fd, UNS_FDOUT, NULL, 0, ns_fd);
-
-   close(ns_fd);
-   return fd;
+   return request_spfs_mngr_sock(NULL);
 }
 
 int spfs_set_mode(int sock, const char *mode)

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH 2/7] spfs: improve error and debug output for spfs_mount()

[Stanislav Kinsburskiy]

Use request_spfs_mngr_sock() for both start and socket request.

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 criu/spfs.c |   15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/criu/spfs.c b/criu/spfs.c
index fff7b9f..c202b14 100644
--- a/criu/spfs.c
+++ b/criu/spfs.c
@@ -294,18 +294,23 @@ int spfs_mount(struct mount_info *mi, const char *source,
 
sock = start_spfs_mngr();
if (sock < 0) {
-   pr_err("failed to mount NFS to path %s\n", mi->mountpoint);
-   return sock;
+   pr_err("failed to connect to SPFS manager: %d\n", sock);
+   ret = sock;
+   goto err;
}
-
ret = spfs_request_mount(sock, mi, source, filesystemtype, mountflags);
close(sock);
if (ret) {
-   pr_err("mount of %s (%s) failed: %d\n", source, filesystemtype, 
ret);
-   return ret;
+   pr_err("mount request for %s (%s) failed: %d\n",
+   source, filesystemtype, ret);
+   goto err;
}
 
return 0;
+
+err:
+   pr_err("failed to mount NFS to path %s\n", mi->mountpoint);
+   return ret;
 }
 
 int spfs_set_env(void)

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH 4/7] spfs: improve SPFS manager start debug and error output

[Stanislav Kinsburskiy]

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 criu/spfs.c |   12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/criu/spfs.c b/criu/spfs.c
index be3bca4..d3391e2 100644
--- a/criu/spfs.c
+++ b/criu/spfs.c
@@ -127,6 +127,8 @@ static int start_spfs_manager(void)
char *socket_path = spfs_manager_socket_path();
int err = -ENOMEM, sock;
 
+   pr_info("Starting SPFS manager\n");
+
err = cr_system(-1, -1, -1, spfs_manager,
(char *[]){ "spfs-manager", "-",
 "-d",
@@ -135,13 +137,17 @@ static int start_spfs_manager(void)
 "--log-dir", spfs_manager_log_dir(),
 "--exit-with-spfs", NULL },
0);
-   pr_info("%s: spfs manager start result: %d\n", __func__, err);
-   if (err)
+   if (err) {
+   pr_err("failed to start SPFS manager binary: %d\n", err);
return err;
+   }
 
sock = sock_seqpacket_connect(socket_path);
-   if (sock < 0)
+   if (sock < 0) {
+   pr_err("failed to connect to SPFS manager via %s: %d\n",
+   socket_path, err);
return sock;
+   }
 
err = install_service_fd(SPFS_MNGR_SK, sock);
if (err < 0) {

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH 5/7] spfs: improve prints in spfs_set_mode() and spfs_release_replace()

[Stanislav Kinsburskiy]

To make them more grepable.

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 criu/spfs.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/criu/spfs.c b/criu/spfs.c
index d3391e2..da19179 100644
--- a/criu/spfs.c
+++ b/criu/spfs.c
@@ -381,9 +381,9 @@ int spfs_set_mode(int sock, const char *mode)
 
ret = spfs_send_request(sock, req, strlen(req) + 1);
if (ret)
-   pr_err("set mode to %s request failed: %d\n", mode, ret);
+   pr_err("set SPFS mode to %s request failed: %d\n", mode, ret);
else
-   pr_debug("Set mode to %s request succeeded\n", mode);
+   pr_debug("Set SPFS mode to %s request succeeded\n", mode);
 
free(req);
return 0;
@@ -396,9 +396,9 @@ int spfs_release_replace(int sock)
 
ret = spfs_send_request(sock, req, strlen(req) + 1);
if (ret)
-   pr_err("release replace request failed: %d\n", ret);
+   pr_err("SPFS release replace request failed: %d\n", ret);
else
-   pr_debug("Release replace request succeeded\n");
+   pr_debug("SPFS \"release replace\" request succeeded\n");
 
return 0;
 }

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH 0/7] spfs: duplicate socket before sending it from usernsd

[Stanislav Kinsburskiy]

Usernds closes socket once it's sent.
So, id should be duplicated before sending, if the socket is expected to be
send multiple times.

https://jira.sw.ru/browse/PSBM-79462

The following series implements...

---

Stanislav Kinsburskiy (7):
  spfs: introduce request_spfs_mngr_sock)() helper
  spfs: improve error and debug output for spfs_mount()
  spfs: remove redundant spfs_service_fd() helper
  spfs: improve SPFS manager start debug and error output
  spfs: improve prints in spfs_set_mode() and spfs_release_replace()
  spfs: return duplicated socket from usernsd
  spfs: switch mountsmode to STUB after root yard depopulation


 criu/cr-restore.c |   12 +++
 criu/spfs.c   |   97 -
 2 files changed, 64 insertions(+), 45 deletions(-)
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH v2] cgroup/cpuset: emulate cgroup in container

[Stanislav Kinsburskiy]

Any changes to this cgroup are skipped in container, but success code is
returned.
The idea is to fool Docker/Kubernetes.

https://jira.sw.ru/browse/PSBM-58423

This patch obsoletes "ve/proc/cpuset: do not show cpuset in CT"

v2:
Do not attach tasks in cpuset_change_cpumask on cpuset set change, it
requested from non-super VE.
This is a second part of the logic.
The first was to not change cpuset for newly added task. This one - to not
set new cpuset for all the tasks in cgroup.

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 kernel/cpuset.c |   12 
 1 file changed, 12 insertions(+)

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index 26d88eb..43b1410 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -848,6 +848,9 @@ static int cpuset_test_cpumask(struct task_struct *tsk,
 static void cpuset_change_cpumask(struct task_struct *tsk,
  struct cgroup_scanner *scan)
 {
+   if (!ve_is_super(get_exec_env()))
+   return;
+
set_cpus_allowed_ptr(tsk, ((cgroup_cs(scan->cg))->cpus_allowed));
 }
 
@@ -1441,6 +1444,9 @@ static int cpuset_can_attach(struct cgroup *cgrp, struct 
cgroup_taskset *tset)
struct task_struct *task;
int ret;
 
+   if (!ve_is_super(get_exec_env()))
+   return 0;
+
mutex_lock(_mutex);
 
ret = -ENOSPC;
@@ -1470,6 +1476,9 @@ static int cpuset_can_attach(struct cgroup *cgrp, struct 
cgroup_taskset *tset)
 static void cpuset_cancel_attach(struct cgroup *cgrp,
 struct cgroup_taskset *tset)
 {
+   if (!ve_is_super(get_exec_env()))
+   return;
+
mutex_lock(_mutex);
cgroup_cs(cgrp)->attach_in_progress--;
mutex_unlock(_mutex);
@@ -1494,6 +1503,9 @@ static void cpuset_attach(struct cgroup *cgrp, struct 
cgroup_taskset *tset)
struct cpuset *cs = cgroup_cs(cgrp);
struct cpuset *oldcs = cgroup_cs(oldcgrp);
 
+   if (!ve_is_super(get_exec_env()))
+   return;
+
mutex_lock(_mutex);
 
/* prepare for attach */

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] cgroup/cpuset: emulate cgroup in container

[Stanislav Kinsburskiy]

Nice catch, thanks!
I'll first try to tweak this gently, so it will look like cpuset cgroup works, 
but it won't.

13.12.2017 16:18, Pavel Tikhomirov пишет:
> So change to cpuset.cpus is actually applied to the task, that's what I mean. 
> Correct me if I'm wrong.
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] cgroup/cpuset: emulate cgroup in container

[Stanislav Kinsburskiy]

Hi Pavel, please, see my comments/question below

13.12.2017 13:43, Pavel Tikhomirov пишет:
> Personally I don't like these as we still have no unswer to "If cpusets are 
> optional for docker, why k8s can't work without them?" it seem there is not 
> enough explanation in VZAP-31.
> 

Well... I have to admit that I don't like it either.
I (frankly) don't like the whole idea of putting kuber into container.
And the reason is so simple: CT is a cheating technique. Those, who like 
"Russian dolls" should use VMs with nested virtualization.
But the truth is that we do care about our customers (not sure why, since they 
pay us less and less).
And because of this we've put so much various sh*t into our kernel already, 
that I got tired to trow it away on each major rebase a long time ago.
But this all is lyrics.
There is a task - and that's the fix.

> We also need to revert the patch below to show cpuset in CT:
> commit 5160bd34c9bd ("ve/proc/cpuset: do not show cpuset in CT")
> 

Sure! It's mentioned in the patch description.

> It seem I can still attach a process to a nested cgroup in CT with these 
> patch:
> 
> CT-6ecd9be1 /# cat /proc/cgroups | grep cpuset
> cpuset    16    1    1
> CT-6ecd9be1 /# ls /sys/fs/cgroup/cpuset/cpuset.cpus
> /sys/fs/cgroup/cpuset/cpuset.cpus
> CT-6ecd9be1 /# mkdir /sys/fs/cgroup/cpuset/test
> CT-6ecd9be1 /# sleep 1000 &
> [1] 678
> CT-6ecd9be1 /# echo  678 > /sys/fs/cgroup/cpuset/test/tasks
> CT-6ecd9be1 /# cat /sys/fs/cgroup/cpuset/test/tasks
> 678
> 

Isn't it wonderful? :)
Poor me, I have to admit, that I didn't know, that this task will be even 
visible in the nest cgroup... :(
I thought, that emulation would be less effective.

Nevertheless, if you have some better way to solve the issue, please, share.

> On 12/13/2017 01:37 PM, Stanislav Kinsburskiy wrote:
>> Any changes to this cgroup are skipped in container, but success code is
>> returned.
>> The idea is to fool Docker/Kubernetes.
>>
>> https://jira.sw.ru/browse/PSBM-58423
>>
>> This patch obsoletes "ve/proc/cpuset: do not show cpuset in CT"
>>
>> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
>> ---
>>   kernel/cpuset.c |    9 +
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/kernel/cpuset.c b/kernel/cpuset.c
>> index 26d88eb..dfac505 100644
>> --- a/kernel/cpuset.c
>> +++ b/kernel/cpuset.c
>> @@ -1441,6 +1441,9 @@ static int cpuset_can_attach(struct cgroup *cgrp, 
>> struct cgroup_taskset *tset)
>>   struct task_struct *task;
>>   int ret;
>>   +    if (!ve_is_super(get_exec_env()))
>> +    return 0;
>> +
>>   mutex_lock(_mutex);
>>     ret = -ENOSPC;
>> @@ -1470,6 +1473,9 @@ static int cpuset_can_attach(struct cgroup *cgrp, 
>> struct cgroup_taskset *tset)
>>   static void cpuset_cancel_attach(struct cgroup *cgrp,
>>    struct cgroup_taskset *tset)
>>   {
>> +    if (!ve_is_super(get_exec_env()))
>> +    return;
>> +
>>   mutex_lock(_mutex);
>>   cgroup_cs(cgrp)->attach_in_progress--;
>>   mutex_unlock(_mutex);
>> @@ -1494,6 +1500,9 @@ static void cpuset_attach(struct cgroup *cgrp, struct 
>> cgroup_taskset *tset)
>>   struct cpuset *cs = cgroup_cs(cgrp);
>>   struct cpuset *oldcs = cgroup_cs(oldcgrp);
>>   +    if (!ve_is_super(get_exec_env()))
>> +    return;
>> +
>>   mutex_lock(_mutex);
>>     /* prepare for attach */
>>
> 
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] cgroup/cpuset: emulate cgroup in container

[Stanislav Kinsburskiy]

Any changes to this cgroup are skipped in container, but success code is
returned.
The idea is to fool Docker/Kubernetes.

https://jira.sw.ru/browse/PSBM-58423

This patch obsoletes "ve/proc/cpuset: do not show cpuset in CT"

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 kernel/cpuset.c |9 +
 1 file changed, 9 insertions(+)

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index 26d88eb..dfac505 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -1441,6 +1441,9 @@ static int cpuset_can_attach(struct cgroup *cgrp, struct 
cgroup_taskset *tset)
struct task_struct *task;
int ret;
 
+   if (!ve_is_super(get_exec_env()))
+   return 0;
+
mutex_lock(_mutex);
 
ret = -ENOSPC;
@@ -1470,6 +1473,9 @@ static int cpuset_can_attach(struct cgroup *cgrp, struct 
cgroup_taskset *tset)
 static void cpuset_cancel_attach(struct cgroup *cgrp,
 struct cgroup_taskset *tset)
 {
+   if (!ve_is_super(get_exec_env()))
+   return;
+
mutex_lock(_mutex);
cgroup_cs(cgrp)->attach_in_progress--;
mutex_unlock(_mutex);
@@ -1494,6 +1500,9 @@ static void cpuset_attach(struct cgroup *cgrp, struct 
cgroup_taskset *tset)
struct cpuset *cs = cgroup_cs(cgrp);
struct cpuset *oldcs = cgroup_cs(oldcgrp);
 
+   if (!ve_is_super(get_exec_env()))
+   return;
+
mutex_lock(_mutex);
 
/* prepare for attach */

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] ve: fix container stopped state check

[Stanislav Kinsburskiy]

Checking for empty cgroup is not correct, because init process leaves cgroup
early in do_exit.
This leads to a situation, when container is treated as stopped but its
resources (VEIP for instance) are not yet released.
Which in turn leads to container restart failure due to non-releases VEIP
address.

https://jira.sw.ru/browse/PSBM-78078

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 kernel/ve/ve.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/ve/ve.c b/kernel/ve/ve.c
index b0188c3..c628516 100644
--- a/kernel/ve/ve.c
+++ b/kernel/ve/ve.c
@@ -846,7 +846,7 @@ static int ve_state_read(struct cgroup *cg, struct cftype 
*cft,
 
if (ve->is_running)
seq_puts(m, "RUNNING");
-   else if (!nr_threads_ve(ve))
+   else if (!ve->init_task)
seq_puts(m, "STOPPED");
else if (ve->ve_ns)
seq_puts(m, "STOPPING");

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] venet: remove obsolete tgt_veip variable

[Stanislav Kinsburskiy]

This was used for "vzredir" feature in vz6 and not used anymore.

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 drivers/net/venetdev.c |   12 +++-
 include/linux/venet.h  |1 -
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/net/venetdev.c b/drivers/net/venetdev.c
index 11f4a66..dffebdc 100644
--- a/drivers/net/venetdev.c
+++ b/drivers/net/venetdev.c
@@ -256,8 +256,7 @@ static void __veip_stop(struct ve_struct *ve)
ptr = list_entry(p, struct ip_entry_struct, ve_list);
ptr->active_env = NULL;
 
-   if (ptr->tgt_veip == NULL)
-   ip_entry_unhash(ptr);
+   ip_entry_unhash(ptr);
}
 
veip_pool_ops->veip_release(ve);
@@ -277,8 +276,6 @@ static int veip_entry_conflict(struct ip_entry_struct 
*entry, struct ve_struct *
 {
if (entry->active_env != NULL)
return -EADDRINUSE;
-   if (entry->tgt_veip && entry->tgt_veip->veid != ve->veid)
-   return -EADDRNOTAVAIL;
 
entry->active_env = ve;
return 0;
@@ -340,8 +337,7 @@ static int veip_entry_del(struct ve_struct *ve, struct 
ve_addr_struct *addr)
err = 0;
found->active_env = NULL;
 
-   if (found->tgt_veip == NULL)
-   ip_entry_unhash(found);
+   ip_entry_unhash(found);
 out:
spin_unlock(_lock);
return err;
@@ -891,7 +887,6 @@ static int veip_seq_show(struct seq_file *m, void *v)
 {
struct hlist_node *p;
struct ip_entry_struct *entry;
-   struct veip_struct *veip;
char s[40];
 
if (v == SEQ_START_TOKEN) {
@@ -902,8 +897,7 @@ static int veip_seq_show(struct seq_file *m, void *v)
p = (struct hlist_node *)v;
entry = hlist_entry(p, struct ip_entry_struct, ip_hash);
veaddr_print(s, sizeof(s), >addr);
-   veip = ACCESS_ONCE(entry->tgt_veip);
-   seq_printf(m, "%39s %10u\n", s, veip == NULL ? 0 : veip->veid);
+   seq_printf(m, "%39s 0\n", s);
return 0;
 }
 
diff --git a/include/linux/venet.h b/include/linux/venet.h
index 7562996..08d89bc 100644
--- a/include/linux/venet.h
+++ b/include/linux/venet.h
@@ -28,7 +28,6 @@ struct ip_entry_struct
 {
struct ve_addr_struct   addr;
struct ve_struct*active_env;
-   struct veip_struct  *tgt_veip;
struct hlist_node   ip_hash;
union {
struct list_headve_list;

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] venet: VEIP release debug patch

[Stanislav Kinsburskiy]

Sorry, ignore

27.11.2017 15:48, Stanislav Kinsburskiy пишет:
> Needed to investigate VEIP release - CT stop race.
> 
> https://jira.sw.ru/browse/PSBM-78078
> 
> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
> ---
>  drivers/net/venetdev.c |6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/venetdev.c b/drivers/net/venetdev.c
> index 11f4a66..dcdb51d 100644
> --- a/drivers/net/venetdev.c
> +++ b/drivers/net/venetdev.c
> @@ -256,8 +256,12 @@ static void __veip_stop(struct ve_struct *ve)
>   ptr = list_entry(p, struct ip_entry_struct, ve_list);
>   ptr->active_env = NULL;
>  
> - if (ptr->tgt_veip == NULL)
> + if (ptr->tgt_veip == NULL) {
> + printk("%s: removing IP for ve %d\n", __func__,
> + ptr->tgt_veip->veid);
> + dump_stack();
>   ip_entry_unhash(ptr);
> + }
>   }
>  
>   veip_pool_ops->veip_release(ve);
> 
> ___
> Devel mailing list
> Devel@openvz.org
> https://lists.openvz.org/mailman/listinfo/devel
> 
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] venet: VEIP release debug patch

[Stanislav Kinsburskiy]

Needed to investigate VEIP release - CT stop race.

https://jira.sw.ru/browse/PSBM-78078

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 drivers/net/venetdev.c |6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/venetdev.c b/drivers/net/venetdev.c
index 11f4a66..dcdb51d 100644
--- a/drivers/net/venetdev.c
+++ b/drivers/net/venetdev.c
@@ -256,8 +256,12 @@ static void __veip_stop(struct ve_struct *ve)
ptr = list_entry(p, struct ip_entry_struct, ve_list);
ptr->active_env = NULL;
 
-   if (ptr->tgt_veip == NULL)
+   if (ptr->tgt_veip == NULL) {
+   printk("%s: removing IP for ve %d\n", __func__,
+   ptr->tgt_veip->veid);
+   dump_stack();
ip_entry_unhash(ptr);
+   }
}
 
veip_pool_ops->veip_release(ve);

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] netfilter: kill the fake untracked conntrack objects

[Stanislav Kinsburskiy]

resurrect an old patch from Pablo Neira to remove the untracked objects.

Currently, there are four possible states of an skb wrt. conntrack.

1. No conntrack attached, ct is NULL.
2. Normal (kmem cache allocated) ct attached.
3. a template (kmalloc'd), not in any hash tables at any point in time
4. the 'untracked' conntrack, a percpu nf_conn object, tagged via
IPS_UNTRACKED_BIT in ct->status.

Untracked is supposed to be identical to case 1.  It exists only
so users can check

-m conntrack --ctstate UNTRACKED vs.
-m conntrack --ctstate INVALID

e.g. attempts to set connmark on INVALID or UNTRACKED conntracks is
supposed to be a no-op.

Thus currently we need to check
ct == NULL || nf_ct_is_untracked(ct)

in a lot of places in order to avoid altering untracked objects.

The other consequence of the percpu untracked object is that all
-j NOTRACK (and, later, kfree_skb of such skbs) result in an atomic op
(inc/dec the untracked conntracks refcount).

This adds a new kernel-private ctinfo state, IP_CT_UNTRACKED, to
make the distinction instead.

The (few) places that care about packet invalid (ct is NULL) vs.
packet untracked now need to test ct == NULL vs. ctinfo ==
IP_CT_UNTRACKED,
but all other places can omit the nf_ct_is_untracked() check.

https://jira.sw.ru/browse/PSBM-71153

Signed-off-by: Florian Westphal <f...@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pa...@netfilter.org>
Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 include/net/ip_vs.h|5 +-
 include/net/netfilter/nf_conntrack.h   |   10 -
 include/uapi/linux/netfilter/nf_conntrack_common.h |6 ++-
 net/ipv4/netfilter/nf_dup_ipv4.c   |5 +-
 net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c |5 +-
 net/ipv6/netfilter/nf_dup_ipv6.c   |5 +-
 net/netfilter/nf_conntrack_core.c  |   42 ++--
 net/netfilter/nf_nat_core.c|3 -
 net/netfilter/xt_CT.c  |   21 +-
 net/netfilter/xt_conntrack.c   |   13 +++---
 net/netfilter/xt_state.c   |   13 +++---
 11 files changed, 41 insertions(+), 87 deletions(-)

diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
index e1599ad..69d2a9a 100644
--- a/include/net/ip_vs.h
+++ b/include/net/ip_vs.h
@@ -1540,9 +1540,8 @@ static inline void ip_vs_notrack(struct sk_buff *skb)
 
if (!ct || !nf_ct_is_untracked(ct)) {
nf_conntrack_put(skb->nfct);
-   skb->nfct = _ct_untracked_get()->ct_general;
-   skb->nfctinfo = IP_CT_NEW;
-   nf_conntrack_get(skb->nfct);
+   skb->nfct = NULL;
+   skb->nfctinfo = IP_CT_UNTRACKED;
}
 #endif
 }
diff --git a/include/net/netfilter/nf_conntrack.h 
b/include/net/netfilter/nf_conntrack.h
index edb8911..b278973 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -232,14 +232,6 @@ extern s32 (*nf_ct_nat_offset)(const struct nf_conn *ct,
   enum ip_conntrack_dir dir,
   u32 seq);
 
-/* Fake conntrack entry for untracked connections */
-DECLARE_PER_CPU(struct nf_conn, nf_conntrack_untracked);
-static inline struct nf_conn *nf_ct_untracked_get(void)
-{
-   return &__raw_get_cpu_var(nf_conntrack_untracked);
-}
-void nf_ct_untracked_status_or(unsigned long bits);
-
 /* Iterate over all conntracks: if iter returns true, it's deleted. */
 void nf_ct_iterate_cleanup(struct net *net,
   int (*iter)(struct nf_conn *i, void *data),
@@ -272,7 +264,7 @@ static inline int nf_ct_is_dying(struct nf_conn *ct)
 
 static inline int nf_ct_is_untracked(const struct nf_conn *ct)
 {
-   return test_bit(IPS_UNTRACKED_BIT, >status);
+   return false;
 }
 
 /* Packet is received from loopback */
diff --git a/include/uapi/linux/netfilter/nf_conntrack_common.h 
b/include/uapi/linux/netfilter/nf_conntrack_common.h
index 6d074d1..6c18960 100644
--- a/include/uapi/linux/netfilter/nf_conntrack_common.h
+++ b/include/uapi/linux/netfilter/nf_conntrack_common.h
@@ -28,12 +28,14 @@ enum ip_conntrack_info {
/* only for userspace compatibility */
 #ifndef __KERNEL__
IP_CT_NEW_REPLY = IP_CT_NUMBER,
+#else
+   IP_CT_UNTRACKED = 7,
 #endif
 };
 
 #define NF_CT_STATE_INVALID_BIT(1 << 0)
 #define NF_CT_STATE_BIT(ctinfo)(1 << ((ctinfo) % 
IP_CT_IS_REPLY + 1))
-#define NF_CT_STATE_UNTRACKED_BIT  (1 << (IP_CT_NUMBER + 1))
+#define NF_CT_STATE_UNTRACKED_BIT  (1 << (IP_CT_UNTRACKED + 1))
 
 /* Bitset representing status of connection. */
 enum ip_conntrack_status {
@@ -90,7 +92,7 @@ enum ip_conntrack_status {
IPS_TEMPLATE_BIT = 11,
IPS_TEMPLATE = (1 << IPS_TEMPLATE_BIT),
 
-

[Devel] [PATCH v2] nfs: abort delegation in dying VE

[Stanislav Kinsburskiy]

Don't queue delegation request, if ve init is exiting.

https://jira.sw.ru/browse/PSBM-77061

v2: check ve is dying under rcu_lock()

Inspired-by: Kirill Tkhai <ktk...@virtuozzo.com>
Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 fs/nfs/delegation.c |   18 +-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 66af497..1d1500c 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -189,15 +189,31 @@ void nfs_inode_reclaim_delegation(struct inode *inode, 
struct rpc_cred *cred,
nfs_inode_set_delegation(inode, cred, res);
 }
 
+static bool ve_abort_delegation(struct inode *inode)
+{
+   struct nfs_client *clp = NFS_SERVER(inode)->nfs_client;
+   struct rpc_xprt *xprt;
+   bool abort;
+
+   rcu_read_lock();
+   xprt = rcu_dereference(clp->cl_rpcclient->cl_xprt);
+   abort = xprt->xprt_net->owner_ve->ve_netns == NULL;
+   rcu_read_unlock();
+
+   return abort;
+}
+
 static int nfs_do_return_delegation(struct inode *inode, struct nfs_delegation 
*delegation, int issync)
 {
int res = 0;
 
-   if (!test_bit(NFS_DELEGATION_REVOKED, >flags))
+   if (!test_bit(NFS_DELEGATION_REVOKED, >flags) &&
+   !ve_abort_delegation(inode)) {
res = nfs4_proc_delegreturn(inode,
delegation->cred,
>stateid,
issync);
+   }
nfs_free_delegation(delegation);
return res;
 }

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] nfs: abort delegation in dying VE

[Stanislav Kinsburskiy]

Don't queue delegation request, if ve init is exiting.

https://jira.sw.ru/browse/PSBM-77061

Inspired-by: Kirill Tkhai <ktk...@virtuozzo.com>
Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 fs/nfs/delegation.c |   16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 66af497..2422754 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -189,15 +189,29 @@ void nfs_inode_reclaim_delegation(struct inode *inode, 
struct rpc_cred *cred,
nfs_inode_set_delegation(inode, cred, res);
 }
 
+static bool ve_abort_delegation(struct inode *inode)
+{
+   struct nfs_client *clp = NFS_SERVER(inode)->nfs_client;
+   struct rpc_xprt *xprt;
+
+   rcu_read_lock();
+   xprt = rcu_dereference(clp->cl_rpcclient->cl_xprt);
+   rcu_read_unlock();
+
+   return xprt->xprt_net->owner_ve->ve_netns == NULL;
+}
+
 static int nfs_do_return_delegation(struct inode *inode, struct nfs_delegation 
*delegation, int issync)
 {
int res = 0;
 
-   if (!test_bit(NFS_DELEGATION_REVOKED, >flags))
+   if (!test_bit(NFS_DELEGATION_REVOKED, >flags) &&
+   !ve_abort_delegation(inode)) {
res = nfs4_proc_delegreturn(inode,
delegation->cred,
>stateid,
issync);
+   }
nfs_free_delegation(delegation);
return res;
 }

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH v6 0/2] nfs: fix race between callback shutdown and execution

[Stanislav Kinsburskiy]

The idea is to use mutex for protecting callback execution agains per-net
callback shutdown and destroying all the net-related backchannel requests
before transports destruction.

---

Stanislav Kinsburskiy (2):
  sunrpc: bc_svc_flush_queue_net() helper introduced
  nfs: protect callback execution against per-net callback thread shutdown


 fs/nfs/callback.c  |   20 
 include/linux/sunrpc/svc.h |3 +++
 net/sunrpc/svc.c   |   15 +++
 3 files changed, 38 insertions(+)

--
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH v6 2/2] nfs: protect callback execution against per-net callback thread shutdown

[Stanislav Kinsburskiy]

From: Stanislav Kinsburskiy <skinsbur...@parallels.com>

Here is the race:

CPU #0  CPU#1

cleanup_mnt nfs41_callback_svc (get xprt from the list)
nfs_callback_down   ...
... ...
svc_close_net   ...
... ...
svc_xprt_free   ...
svc_bc_sock_freebc_svc_process
kfree(xprt) svc_process_common
rqstp->rq_xprt->xpt_ops (use after free)

The problem is that per-net SUNRPC transports shutdown is done regardless
current callback execution. This is a race leading to transport use-after-free
in callback handler.
This patch fixes it in stright-forward way. I.e. it protects callback
execution with the same mutex used for per-net data creation and destruction.
Hopefully, it won't slow down NFS client significantly.

https://jira.sw.ru/browse/PSBM-75751

v6: destroy all per-net backchannel requests only for NFSv4.1

v5: destroy all per-net backchannel requests before transports on in
nfs_callback_down_net

v4: use another mutex to protect callback execution agains per-net transports
shutdown.
This guarantees, that transports won't be destroyed by shutdown callback while
execution is in progress and vice versa.

v3: Fix mutex deadlock, when shutdown callback waits for thread to exit (with
mutex taken), while thread wait for the mutex to take.
The idea is to simply check if thread has to exit, if mutex lock has failed.
This is a busy loop, but it shouldn't happend often and for long.

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 fs/nfs/callback.c  |   20 
 include/linux/sunrpc/svc.h |1 +
 2 files changed, 21 insertions(+)

diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index 0beb275..feffccf 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -99,6 +99,8 @@ nfs4_callback_up(struct svc_serv *serv)
 }
 
 #if defined(CONFIG_NFS_V4_1)
+static DEFINE_MUTEX(nfs41_callback_mutex);
+
 /*
  * The callback service for NFSv4.1 callbacks
  */
@@ -117,6 +119,12 @@ nfs41_callback_svc(void *vrqstp)
if (try_to_freeze())
continue;
 
+   mutex_lock(_callback_mutex);
+   if (kthread_should_stop()) {
+   mutex_unlock(_callback_mutex);
+   return 0;
+   }
+
prepare_to_wait(>sv_cb_waitq, , TASK_INTERRUPTIBLE);
spin_lock_bh(>sv_cb_lock);
if (!list_empty(>sv_cb_list)) {
@@ -129,8 +137,10 @@ nfs41_callback_svc(void *vrqstp)
error = bc_svc_process(serv, req, rqstp);
dprintk("bc_svc_process() returned w/ error code= %d\n",
error);
+   mutex_unlock(_callback_mutex);
} else {
spin_unlock_bh(>sv_cb_lock);
+   mutex_unlock(_callback_mutex);
schedule();
finish_wait(>sv_cb_waitq, );
}
@@ -139,6 +149,13 @@ nfs41_callback_svc(void *vrqstp)
return 0;
 }
 
+static void nfs41_callback_down_net(struct svc_serv *serv, struct net *net)
+{
+   mutex_lock(_callback_mutex);
+   bc_svc_flush_queue_net(serv, net);
+   mutex_unlock(_callback_mutex);
+}
+
 /*
  * Bring up the NFSv4.1 callback service
  */
@@ -150,6 +167,7 @@ nfs41_callback_up(struct svc_serv *serv)
INIT_LIST_HEAD(>sv_cb_list);
spin_lock_init(>sv_cb_lock);
init_waitqueue_head(>sv_cb_waitq);
+   serv->svc_cb_down_net = nfs41_callback_down_net;
rqstp = svc_prepare_thread(serv, >sv_pools[0], NUMA_NO_NODE);
dprintk("--> %s return %d\n", __func__, PTR_ERR_OR_ZERO(rqstp));
return rqstp;
@@ -242,6 +260,8 @@ static void nfs_callback_down_net(u32 minorversion, struct 
svc_serv *serv, struc
return;
 
dprintk("NFS: destroy per-net callback data; net=%p\n", net);
+   if (serv->svc_cb_down_net)
+   serv->svc_cb_down_net(serv, net);
svc_shutdown_net(serv, net);
 }
 
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index fe70ff0..c04ef80 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -108,6 +108,7 @@ struct svc_serv {
wait_queue_head_t   sv_cb_waitq;/* sleep here if there are no
 * entries in the svc_cb_list */
struct svc_xprt *sv_bc_xprt;/* callback on fore channel */
+   void(*svc_cb_down_net)(struct svc_serv *serv, 
struct net *net);
 #endif /* CONFIG_SUNRPC_BACKCHANNEL */
 };
 

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH v6 1/2] sunrpc: bc_svc_flush_queue_net() helper introduced

[Stanislav Kinsburskiy]

From: Stanislav Kinsburskiy <skinsbur...@parallels.com>

This helper can be used to remove backchannel requests from callback queue on
per-net basis.

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 include/linux/sunrpc/svc.h |2 ++
 net/sunrpc/svc.c   |   15 +++
 2 files changed, 17 insertions(+)

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 2b30868..fe70ff0 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -484,6 +484,8 @@ void   svc_reserve(struct svc_rqst *rqstp, 
int space);
 struct svc_pool *  svc_pool_for_cpu(struct svc_serv *serv, int cpu);
 char *svc_print_addr(struct svc_rqst *, char *, size_t);
 
+void bc_svc_flush_queue_net(struct svc_serv *serv, struct net *net);
+
 #defineRPC_MAX_ADDRBUFLEN  (63U)
 
 /*
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index de8cded..2ca4ff7 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1338,6 +1338,21 @@ svc_process(struct svc_rqst *rqstp)
 EXPORT_SYMBOL_GPL(svc_process);
 
 #if defined(CONFIG_SUNRPC_BACKCHANNEL)
+void bc_svc_flush_queue_net(struct svc_serv *serv, struct net *net)
+{
+   struct rpc_rqst *req, *tmp;
+
+   spin_lock_bh(>sv_cb_lock);
+   list_for_each_entry_safe(req, tmp, >sv_cb_list, rq_bc_list) {
+   if (req->rq_xprt->xprt_net == net) {
+   list_del(>rq_bc_list);
+   xprt_free_bc_request(req);
+   }
+   }
+   spin_unlock_bh(>sv_cb_lock);
+}
+EXPORT_SYMBOL_GPL(bc_svc_flush_queue_net);
+
 /*
  * Process a backchannel RPC request that arrived over an existing
  * outbound connection

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH v5 2/2] nfs: protect callback execution against per-net callback thread shutdown

[Stanislav Kinsburskiy]

09.11.2017 10:33, Kirill Tkhai пишет:
> 
> Looks good for me, except there is one more call of nfs_callback_down_net() 
> in nfs_callback_up().
> It's not clear for me, we shouldn't protect it via the mutex too. Should we?
> 

Looks like no, mount is not ready yet and there is no connection to server, 
thus no request from server can
come to the moment and we don't need any additional protection here.
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH v5 2/2] nfs: protect callback execution against per-net callback thread shutdown

[Stanislav Kinsburskiy]

From: Stanislav Kinsburskiy <skinsbur...@parallels.com>

Here is the race:

CPU #0  CPU#1

cleanup_mnt nfs41_callback_svc (get xprt from the list)
nfs_callback_down   ...
... ...
svc_close_net   ...
... ...
svc_xprt_free   ...
svc_bc_sock_freebc_svc_process
kfree(xprt) svc_process_common
rqstp->rq_xprt->xpt_ops (use after free)

The problem is that per-net SUNRPC transports shutdown is done regardless
current callback execution. This is a race leading to transport use-after-free
in callback handler.
This patch fixes it in stright-forward way. I.e. it protects callback
execution with the same mutex used for per-net data creation and destruction.
Hopefully, it won't slow down NFS client significantly.

https://jira.sw.ru/browse/PSBM-75751

v5: destroy all per-net backchannel requests before transports on in
nfs_callback_down_net

v4: use another mutex to protect callback execution agains per-net transports
shutdown.
This guarantees, that transports won't be destroyed by shutdown callback while
execution is in progress and vice versa.

v3: Fix mutex deadlock, when shutdown callback waits for thread to exit (with
mutex taken), while thread wait for the mutex to take.
The idea is to simply check if thread has to exit, if mutex lock has failed.
This is a busy loop, but it shouldn't happend often and for long.

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 fs/nfs/callback.c |   17 +
 1 file changed, 17 insertions(+)

diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index 0beb275..e18d774 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -99,6 +99,8 @@ nfs4_callback_up(struct svc_serv *serv)
 }
 
 #if defined(CONFIG_NFS_V4_1)
+static DEFINE_MUTEX(nfs41_callback_mutex);
+
 /*
  * The callback service for NFSv4.1 callbacks
  */
@@ -117,6 +119,12 @@ nfs41_callback_svc(void *vrqstp)
if (try_to_freeze())
continue;
 
+   mutex_lock(_callback_mutex);
+   if (kthread_should_stop()) {
+   mutex_unlock(_callback_mutex);
+   return 0;
+   }
+
prepare_to_wait(>sv_cb_waitq, , TASK_INTERRUPTIBLE);
spin_lock_bh(>sv_cb_lock);
if (!list_empty(>sv_cb_list)) {
@@ -129,8 +137,10 @@ nfs41_callback_svc(void *vrqstp)
error = bc_svc_process(serv, req, rqstp);
dprintk("bc_svc_process() returned w/ error code= %d\n",
error);
+   mutex_unlock(_callback_mutex);
} else {
spin_unlock_bh(>sv_cb_lock);
+   mutex_unlock(_callback_mutex);
schedule();
finish_wait(>sv_cb_waitq, );
}
@@ -242,6 +252,7 @@ static void nfs_callback_down_net(u32 minorversion, struct 
svc_serv *serv, struc
return;
 
dprintk("NFS: destroy per-net callback data; net=%p\n", net);
+   bc_svc_flush_queue_net(serv, net);
svc_shutdown_net(serv, net);
 }
 
@@ -377,7 +388,13 @@ void nfs_callback_down(int minorversion, struct net *net)
struct nfs_callback_data *cb_info = _callback_info[minorversion];
 
mutex_lock(_callback_mutex);
+#if defined(CONFIG_NFS_V4_1)
+   mutex_lock(_callback_mutex);
+   nfs_callback_down_net(minorversion, cb_info->serv, net);
+   mutex_unlock(_callback_mutex);
+#else
nfs_callback_down_net(minorversion, cb_info->serv, net);
+#endif
cb_info->users--;
if (cb_info->users == 0 && cb_info->task != NULL) {
kthread_stop(cb_info->task);

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH v5 1/2] sunrpc: bc_svc_flush_queue_net() helper introduced

[Stanislav Kinsburskiy]

From: Stanislav Kinsburskiy <skinsbur...@parallels.com>

This helper can be used to remove backchannel requests from callback queue on
per-net basis.

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 include/linux/sunrpc/svc.h |2 ++
 net/sunrpc/svc.c   |   15 +++
 2 files changed, 17 insertions(+)

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 2b30868..fe70ff0 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -484,6 +484,8 @@ void   svc_reserve(struct svc_rqst *rqstp, 
int space);
 struct svc_pool *  svc_pool_for_cpu(struct svc_serv *serv, int cpu);
 char *svc_print_addr(struct svc_rqst *, char *, size_t);
 
+void bc_svc_flush_queue_net(struct svc_serv *serv, struct net *net);
+
 #defineRPC_MAX_ADDRBUFLEN  (63U)
 
 /*
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index de8cded..2ca4ff7 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1338,6 +1338,21 @@ svc_process(struct svc_rqst *rqstp)
 EXPORT_SYMBOL_GPL(svc_process);
 
 #if defined(CONFIG_SUNRPC_BACKCHANNEL)
+void bc_svc_flush_queue_net(struct svc_serv *serv, struct net *net)
+{
+   struct rpc_rqst *req, *tmp;
+
+   spin_lock_bh(>sv_cb_lock);
+   list_for_each_entry_safe(req, tmp, >sv_cb_list, rq_bc_list) {
+   if (req->rq_xprt->xprt_net == net) {
+   list_del(>rq_bc_list);
+   xprt_free_bc_request(req);
+   }
+   }
+   spin_unlock_bh(>sv_cb_lock);
+}
+EXPORT_SYMBOL_GPL(bc_svc_flush_queue_net);
+
 /*
  * Process a backchannel RPC request that arrived over an existing
  * outbound connection

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] nfs: protect callback execution against per-net callback thread shutdown

[Stanislav Kinsburskiy]

Please, ignore

08.11.2017 15:07, Stanislav Kinsburskiy пишет:
> From: Stanislav Kinsburskiy <skinsbur...@parallels.com>
> 
> Here is the race:
> 
> CPU #0CPU#1
> 
> cleanup_mnt   nfs41_callback_svc (get xprt from the list)
> nfs_callback_down ...
> ...   ...
> svc_close_net ...
> ...   ...
> svc_xprt_free ...
> svc_bc_sock_free  bc_svc_process
> kfree(xprt)   svc_process_common
>   rqstp->rq_xprt->xpt_ops (use after free)
> 
> The problem is that per-net SUNRPC transports shutdown is done regardless
> current callback execution. This is a race leading to transport use-after-free
> in callback handler.
> This patch fixes it in stright-forward way. I.e. it protects callback
> execution with the same mutex used for per-net data creation and destruction.
> Hopefully, it won't slow down NFS client significantly.
> 
> https://jira.sw.ru/browse/PSBM-75751
> 
> v4: use another mutex to protect callback execution agains per-net transports
> shutdown.
> This guarantees, that transports won't be destroyed by shutdown callback while
> execution is in progress and vice versa.
> 
> v3: Fix mutex deadlock, when shutdown callback waits for thread to exit (with
> mutex taken), while thread wait for the mutex to take.
> The idea is to simply check if thread has to exit, if mutex lock has failed.
> This is a busy loop, but it shouldn't happend often and for long.
> 
> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@parallels.com>
> ---
>  fs/nfs/callback.c |   16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> index 0beb275..4d7979b 100644
> --- a/fs/nfs/callback.c
> +++ b/fs/nfs/callback.c
> @@ -99,6 +99,8 @@ nfs4_callback_up(struct svc_serv *serv)
>  }
>  
>  #if defined(CONFIG_NFS_V4_1)
> +static DEFINE_MUTEX(nfs41_callback_mutex);
> +
>  /*
>   * The callback service for NFSv4.1 callbacks
>   */
> @@ -117,6 +119,12 @@ nfs41_callback_svc(void *vrqstp)
>   if (try_to_freeze())
>   continue;
>  
> + mutex_lock(_callback_mutex);
> + if (kthread_should_stop()) {
> + mutex_unlock(_callback_mutex);
> + return 0;
> + }
> +
>   prepare_to_wait(>sv_cb_waitq, , TASK_INTERRUPTIBLE);
>   spin_lock_bh(>sv_cb_lock);
>   if (!list_empty(>sv_cb_list)) {
> @@ -129,8 +137,10 @@ nfs41_callback_svc(void *vrqstp)
>   error = bc_svc_process(serv, req, rqstp);
>   dprintk("bc_svc_process() returned w/ error code= %d\n",
>   error);
> + mutex_unlock(_callback_mutex);
>   } else {
>   spin_unlock_bh(>sv_cb_lock);
> + mutex_unlock(_callback_mutex);
>   schedule();
>   finish_wait(>sv_cb_waitq, );
>   }
> @@ -377,7 +387,13 @@ void nfs_callback_down(int minorversion, struct net *net)
>   struct nfs_callback_data *cb_info = _callback_info[minorversion];
>  
>   mutex_lock(_callback_mutex);
> +#if defined(CONFIG_NFS_V4_1)
> + mutex_lock(_callback_mutex);
> + nfs_callback_down_net(minorversion, cb_info->serv, net);
> + mutex_unlock(_callback_mutex);
> +#else
>   nfs_callback_down_net(minorversion, cb_info->serv, net);
> +#endif
>   cb_info->users--;
>   if (cb_info->users == 0 && cb_info->task != NULL) {
>   kthread_stop(cb_info->task);
> 
> ___
> Devel mailing list
> Devel@openvz.org
> https://lists.openvz.org/mailman/listinfo/devel
> 
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH v4] nfs: protect callback execution against per-net callback thread shutdown

[Stanislav Kinsburskiy]

From: Stanislav Kinsburskiy <skinsbur...@parallels.com>

Here is the race:

CPU #0  CPU#1

cleanup_mnt nfs41_callback_svc (get xprt from the list)
nfs_callback_down   ...
... ...
svc_close_net   ...
... ...
svc_xprt_free   ...
svc_bc_sock_freebc_svc_process
kfree(xprt) svc_process_common
rqstp->rq_xprt->xpt_ops (use after free)

The problem is that per-net SUNRPC transports shutdown is done regardless
current callback execution. This is a race leading to transport use-after-free
in callback handler.
This patch fixes it in stright-forward way. I.e. it protects callback
execution with the same mutex used for per-net data creation and destruction.
Hopefully, it won't slow down NFS client significantly.

https://jira.sw.ru/browse/PSBM-75751

v4: use another mutex to protect callback execution agains per-net transports
shutdown.
This guarantees, that transports won't be destroyed by shutdown callback while
execution is in progress and vice versa.

v3: Fix mutex deadlock, when shutdown callback waits for thread to exit (with
mutex taken), while thread wait for the mutex to take.
The idea is to simply check if thread has to exit, if mutex lock has failed.
This is a busy loop, but it shouldn't happend often and for long.

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@parallels.com>
---
 fs/nfs/callback.c |   16 
 1 file changed, 16 insertions(+)

diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index 0beb275..4d7979b 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -99,6 +99,8 @@ nfs4_callback_up(struct svc_serv *serv)
 }
 
 #if defined(CONFIG_NFS_V4_1)
+static DEFINE_MUTEX(nfs41_callback_mutex);
+
 /*
  * The callback service for NFSv4.1 callbacks
  */
@@ -117,6 +119,12 @@ nfs41_callback_svc(void *vrqstp)
if (try_to_freeze())
continue;
 
+   mutex_lock(_callback_mutex);
+   if (kthread_should_stop()) {
+   mutex_unlock(_callback_mutex);
+   return 0;
+   }
+
prepare_to_wait(>sv_cb_waitq, , TASK_INTERRUPTIBLE);
spin_lock_bh(>sv_cb_lock);
if (!list_empty(>sv_cb_list)) {
@@ -129,8 +137,10 @@ nfs41_callback_svc(void *vrqstp)
error = bc_svc_process(serv, req, rqstp);
dprintk("bc_svc_process() returned w/ error code= %d\n",
error);
+   mutex_unlock(_callback_mutex);
} else {
spin_unlock_bh(>sv_cb_lock);
+   mutex_unlock(_callback_mutex);
schedule();
finish_wait(>sv_cb_waitq, );
}
@@ -377,7 +387,13 @@ void nfs_callback_down(int minorversion, struct net *net)
struct nfs_callback_data *cb_info = _callback_info[minorversion];
 
mutex_lock(_callback_mutex);
+#if defined(CONFIG_NFS_V4_1)
+   mutex_lock(_callback_mutex);
+   nfs_callback_down_net(minorversion, cb_info->serv, net);
+   mutex_unlock(_callback_mutex);
+#else
nfs_callback_down_net(minorversion, cb_info->serv, net);
+#endif
cb_info->users--;
if (cb_info->users == 0 && cb_info->task != NULL) {
kthread_stop(cb_info->task);

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] nfs: protect callback execution against per-net callback thread shutdown

[Stanislav Kinsburskiy]

From: Stanislav Kinsburskiy <skinsbur...@parallels.com>

Here is the race:

CPU #0  CPU#1

cleanup_mnt nfs41_callback_svc (get xprt from the list)
nfs_callback_down   ...
... ...
svc_close_net   ...
... ...
svc_xprt_free   ...
svc_bc_sock_freebc_svc_process
kfree(xprt) svc_process_common
rqstp->rq_xprt->xpt_ops (use after free)

The problem is that per-net SUNRPC transports shutdown is done regardless
current callback execution. This is a race leading to transport use-after-free
in callback handler.
This patch fixes it in stright-forward way. I.e. it protects callback
execution with the same mutex used for per-net data creation and destruction.
Hopefully, it won't slow down NFS client significantly.

https://jira.sw.ru/browse/PSBM-75751

v4: use another mutex to protect callback execution agains per-net transports
shutdown.
This guarantees, that transports won't be destroyed by shutdown callback while
execution is in progress and vice versa.

v3: Fix mutex deadlock, when shutdown callback waits for thread to exit (with
mutex taken), while thread wait for the mutex to take.
The idea is to simply check if thread has to exit, if mutex lock has failed.
This is a busy loop, but it shouldn't happend often and for long.

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@parallels.com>
---
 fs/nfs/callback.c |   16 
 1 file changed, 16 insertions(+)

diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index 0beb275..4d7979b 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -99,6 +99,8 @@ nfs4_callback_up(struct svc_serv *serv)
 }
 
 #if defined(CONFIG_NFS_V4_1)
+static DEFINE_MUTEX(nfs41_callback_mutex);
+
 /*
  * The callback service for NFSv4.1 callbacks
  */
@@ -117,6 +119,12 @@ nfs41_callback_svc(void *vrqstp)
if (try_to_freeze())
continue;
 
+   mutex_lock(_callback_mutex);
+   if (kthread_should_stop()) {
+   mutex_unlock(_callback_mutex);
+   return 0;
+   }
+
prepare_to_wait(>sv_cb_waitq, , TASK_INTERRUPTIBLE);
spin_lock_bh(>sv_cb_lock);
if (!list_empty(>sv_cb_list)) {
@@ -129,8 +137,10 @@ nfs41_callback_svc(void *vrqstp)
error = bc_svc_process(serv, req, rqstp);
dprintk("bc_svc_process() returned w/ error code= %d\n",
error);
+   mutex_unlock(_callback_mutex);
} else {
spin_unlock_bh(>sv_cb_lock);
+   mutex_unlock(_callback_mutex);
schedule();
finish_wait(>sv_cb_waitq, );
}
@@ -377,7 +387,13 @@ void nfs_callback_down(int minorversion, struct net *net)
struct nfs_callback_data *cb_info = _callback_info[minorversion];
 
mutex_lock(_callback_mutex);
+#if defined(CONFIG_NFS_V4_1)
+   mutex_lock(_callback_mutex);
+   nfs_callback_down_net(minorversion, cb_info->serv, net);
+   mutex_unlock(_callback_mutex);
+#else
nfs_callback_down_net(minorversion, cb_info->serv, net);
+#endif
cb_info->users--;
if (cb_info->users == 0 && cb_info->task != NULL) {
kthread_stop(cb_info->task);

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH v3] nfs: protect callback execution against per-net callback thread shutdown

[Stanislav Kinsburskiy]

Please, see my comments below.

07.11.2017 12:30, Kirill Tkhai пишет:
> On 07.11.2017 13:39, Stanislav Kinsburskiy wrote:
>> From: Stanislav Kinsburskiy <skinsbur...@parallels.com>
>>
>> Here is the race:
>>
>> CPU #0   CPU#1
>>
>> cleanup_mnt  nfs41_callback_svc (get xprt from the list)
>> nfs_callback_down...
>> ...  ...
>> svc_close_net...
>> ...  ...
>> svc_xprt_free...
>> svc_bc_sock_free bc_svc_process
>> kfree(xprt)  svc_process_common
>>  rqstp->rq_xprt->xpt_ops (use after free)
>>
>> The problem is that per-net SUNRPC transports shutdown is done regardless
>> current callback execution. This is a race leading to transport 
>> use-after-free
>> in callback handler.
>> This patch fixes it in stright-forward way. I.e. it protects callback
>> execution with the same mutex used for per-net data creation and destruction.
>> Hopefully, it won't slow down NFS client significantly.
>>
>> https://jira.sw.ru/browse/PSBM-75751
>>
>> v3: Fix mutex deadlock, when shutdown callback waits for thread to exit (with
>> mutex taken), while thread wait for the mutex to take.
>> The idea is to simply check if thread has to exit, if mutex lock has failed.
>> This is a busy loop, but it shouldn't happend often and for long.
>>
>> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@parallels.com>
>> ---
>>  fs/nfs/callback.c |6 ++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
>> index 0beb275..bbb07e4 100644
>> --- a/fs/nfs/callback.c
>> +++ b/fs/nfs/callback.c
>> @@ -117,7 +117,11 @@ nfs41_callback_svc(void *vrqstp)
>>  if (try_to_freeze())
>>  continue;
>>  
>> +if (!mutex_trylock(_callback_mutex))
>> +   continue;
> 
> This looks like a busy loop (that especially bad, because mutex-owner may 
> sleep
> at the moment we are looping).
> 

Well, yes. It a busy loop. And that's not good. But I have to mention, that 
this busy loop can happen only on umount request.

> It seems the solution may be to change nfs_callback_down() function.
> Can't we flush pending request in nfs_callback_down()?

Well, this already happens. The problem is in race between transport usage 
(unconditional) and its destruction (also unconditional).
IOW, there should be either reference counting or some critical section.
Looks like the former will need a way more code and thus more error-prone, the 
latter is uglier, but code should be simpler at first sight.


> Or just delete it from sv_cb_list without handling (does nfs proto allow 
> that)?
> 

Well, this looks like a promising idea. But yet again, there should be some 
protection against transport usage in shutdown helper.
And it's not yet clear, how to implement it without significant code change...

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH v3] nfs: protect callback execution against per-net callback thread shutdown

[Stanislav Kinsburskiy]

From: Stanislav Kinsburskiy <skinsbur...@parallels.com>

Here is the race:

CPU #0  CPU#1

cleanup_mnt nfs41_callback_svc (get xprt from the list)
nfs_callback_down   ...
... ...
svc_close_net   ...
... ...
svc_xprt_free   ...
svc_bc_sock_freebc_svc_process
kfree(xprt) svc_process_common
rqstp->rq_xprt->xpt_ops (use after free)

The problem is that per-net SUNRPC transports shutdown is done regardless
current callback execution. This is a race leading to transport use-after-free
in callback handler.
This patch fixes it in stright-forward way. I.e. it protects callback
execution with the same mutex used for per-net data creation and destruction.
Hopefully, it won't slow down NFS client significantly.

https://jira.sw.ru/browse/PSBM-75751

v3: Fix mutex deadlock, when shutdown callback waits for thread to exit (with
mutex taken), while thread wait for the mutex to take.
The idea is to simply check if thread has to exit, if mutex lock has failed.
This is a busy loop, but it shouldn't happend often and for long.

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@parallels.com>
---
 fs/nfs/callback.c |6 ++
 1 file changed, 6 insertions(+)

diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index 0beb275..bbb07e4 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -117,7 +117,11 @@ nfs41_callback_svc(void *vrqstp)
if (try_to_freeze())
continue;
 
+   if (!mutex_trylock(_callback_mutex))
+  continue;
+
prepare_to_wait(>sv_cb_waitq, , TASK_INTERRUPTIBLE);
+
spin_lock_bh(>sv_cb_lock);
if (!list_empty(>sv_cb_list)) {
req = list_first_entry(>sv_cb_list,
@@ -129,8 +133,10 @@ nfs41_callback_svc(void *vrqstp)
error = bc_svc_process(serv, req, rqstp);
dprintk("bc_svc_process() returned w/ error code= %d\n",
error);
+   mutex_unlock(_callback_mutex);
} else {
spin_unlock_bh(>sv_cb_lock);
+   mutex_unlock(_callback_mutex);
schedule();
finish_wait(>sv_cb_waitq, );
}

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH v2] nfs: protect callback execution against per-net callback thread shutdown

[Stanislav Kinsburskiy]

Sorry, this one is bad as well.

07.11.2017 11:28, Stanislav Kinsburskiy пишет:
> Here is the race:
> 
> CPU #0CPU#1
> 
> cleanup_mnt   nfs41_callback_svc (get xprt from the list)
> nfs_callback_down ...
> ...   ...
> svc_close_net ...
> ...   ...
> svc_xprt_free ...
> svc_bc_sock_free  bc_svc_process
> kfree(xprt)   svc_process_common
>   rqstp->rq_xprt->xpt_ops (use after free)
> 
> The problem is that per-net SUNRPC transports shutdown is done regardless
> current callback execution. This is a race leading to transport use-after-free
> in callback handler.
> This patch fixes it in stright-forward way. I.e. it protects callback
> execution with the same mutex used for per-net data creation and destruction.
> Hopefully, it won't slow down NFS client significantly.
> 
> https://jira.sw.ru/browse/PSBM-75751
> 
> v2: Fix mutex deadlock, when shutdown callback waits for thread to exit (with
> mutex taken), while thread wait for the mutex to take.
> 
> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
> ---
>  fs/nfs/callback.c |7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> index 0beb275..eed861a 100644
> --- a/fs/nfs/callback.c
> +++ b/fs/nfs/callback.c
> @@ -118,6 +118,11 @@ nfs41_callback_svc(void *vrqstp)
>   continue;
>  
>   prepare_to_wait(>sv_cb_waitq, , TASK_INTERRUPTIBLE);
> +
> + if (!mutex_trylock(_callback_mutex) &&
> + kthread_should_stop())
> + return 0;
> +
>   spin_lock_bh(>sv_cb_lock);
>   if (!list_empty(>sv_cb_list)) {
>   req = list_first_entry(>sv_cb_list,
> @@ -129,8 +134,10 @@ nfs41_callback_svc(void *vrqstp)
>   error = bc_svc_process(serv, req, rqstp);
>   dprintk("bc_svc_process() returned w/ error code= %d\n",
>   error);
> + mutex_unlock(_callback_mutex);
>   } else {
>   spin_unlock_bh(>sv_cb_lock);
> + mutex_unlock(_callback_mutex);
>   schedule();
>   finish_wait(>sv_cb_waitq, );
>   }
> 
> ___
> Devel mailing list
> Devel@openvz.org
> https://lists.openvz.org/mailman/listinfo/devel
> 
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH v2] nfs: protect callback execution against per-net callback thread shutdown

[Stanislav Kinsburskiy]

Here is the race:

CPU #0  CPU#1

cleanup_mnt nfs41_callback_svc (get xprt from the list)
nfs_callback_down   ...
... ...
svc_close_net   ...
... ...
svc_xprt_free   ...
svc_bc_sock_freebc_svc_process
kfree(xprt) svc_process_common
rqstp->rq_xprt->xpt_ops (use after free)

The problem is that per-net SUNRPC transports shutdown is done regardless
current callback execution. This is a race leading to transport use-after-free
in callback handler.
This patch fixes it in stright-forward way. I.e. it protects callback
execution with the same mutex used for per-net data creation and destruction.
Hopefully, it won't slow down NFS client significantly.

https://jira.sw.ru/browse/PSBM-75751

v2: Fix mutex deadlock, when shutdown callback waits for thread to exit (with
mutex taken), while thread wait for the mutex to take.

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 fs/nfs/callback.c |7 +++
 1 file changed, 7 insertions(+)

diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index 0beb275..eed861a 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -118,6 +118,11 @@ nfs41_callback_svc(void *vrqstp)
continue;
 
prepare_to_wait(>sv_cb_waitq, , TASK_INTERRUPTIBLE);
+
+   if (!mutex_trylock(_callback_mutex) &&
+   kthread_should_stop())
+   return 0;
+
spin_lock_bh(>sv_cb_lock);
if (!list_empty(>sv_cb_list)) {
req = list_first_entry(>sv_cb_list,
@@ -129,8 +134,10 @@ nfs41_callback_svc(void *vrqstp)
error = bc_svc_process(serv, req, rqstp);
dprintk("bc_svc_process() returned w/ error code= %d\n",
error);
+   mutex_unlock(_callback_mutex);
} else {
spin_unlock_bh(>sv_cb_lock);
+   mutex_unlock(_callback_mutex);
schedule();
finish_wait(>sv_cb_waitq, );
}

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] nfs: protect callback execution against per-net callback thread shutdown

[Stanislav Kinsburskiy]

07.11.2017 11:03, Kirill Tkhai пишет:
>>> Couldn't this change introduce a deadlock like below?
>>>   [thread]
>>> nfs_callback_down()   
>>> nfs41_callback_svc()
>>>mutex_lock(_callback_mutex);   
>>>kthread_stop(cb_info->task);   
>>>   wake_up_process();  
>>>   wait_for_completion(>exited);  
> 
> And what about above one?
> 

Good catch. Need to think more about it then.
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] nfs: protect callback execution against per-net callback thread shutdown

[Stanislav Kinsburskiy]

Sure, here it is:

CPU #0  CPU#1

cleanup_mnt nfs41_callback_svc
... (get transport from the list)
nfs_callback_down   ...
... ...
svc_close_net   ...
... ...
svc_xprt_free   ...
svc_bc_sock_freebc_svc_process
kfree(xprt) svc_process_common
rqstp->rq_xprt->xpt_ops (use 
after free)



07.11.2017 10:49, Kirill Tkhai пишет:
> On 03.11.2017 19:47, Stanislav Kinsburskiy wrote:
>> From: Stanislav Kinsburskiy <skinsbur...@parallels.com>
>>
>> The problem is that per-net SUNRPC transports shutdown is done regardless
>> current callback execution. This is a race leading to transport 
>> use-after-free
>> in callback handler.
> 
> Could you please draw the race to show the interaction between functions?
> 
>> This patch fixes it in stright-forward way. I.e. it protects callback
>> execution with the same mutex used for per-net data creation and destruction.
>> Hopefully, it won't slow down NFS client significantly.
>>
>> https://jira.sw.ru/browse/PSBM-75751
>>
>> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@parallels.com>
>> ---
>>  fs/nfs/callback.c |3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
>> index 0beb275..82e8ed1 100644
>> --- a/fs/nfs/callback.c
>> +++ b/fs/nfs/callback.c
>> @@ -118,6 +118,7 @@ nfs41_callback_svc(void *vrqstp)
>>  continue;
>>  
>>  prepare_to_wait(>sv_cb_waitq, , TASK_INTERRUPTIBLE);
>> +mutex_lock(_callback_mutex);
>>  spin_lock_bh(>sv_cb_lock);
>>  if (!list_empty(>sv_cb_list)) {
>>  req = list_first_entry(>sv_cb_list,
>> @@ -129,8 +130,10 @@ nfs41_callback_svc(void *vrqstp)
>>  error = bc_svc_process(serv, req, rqstp);
>>  dprintk("bc_svc_process() returned w/ error code= %d\n",
>>  error);
>> +mutex_unlock(_callback_mutex);
>>  } else {
>>  spin_unlock_bh(>sv_cb_lock);
>> +mutex_unlock(_callback_mutex);
>>  schedule();
>>  finish_wait(>sv_cb_waitq, );
>>  }
> 
> Couldn't this change introduce a deadlock like below?
>   [thread]
> nfs_callback_down()   nfs41_callback_svc()
>mutex_lock(_callback_mutex);   
>kthread_stop(cb_info->task);   
>   wake_up_process();  
>   wait_for_completion(>exited);  
> 
>   
> mutex_lock(_callback_mutex); 
> 
> 
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] nfs: protect callback execution against per-net callback thread shutdown

[Stanislav Kinsburskiy]

Well... Maybe.
Let's check, how it works with our kernel.

07.11.2017 10:16, Konstantin Khorenko пишет:
> Going to send it to mainstream as well?
> 
> -- 
> Best regards,
> 
> Konstantin Khorenko,
> Virtuozzo Linux Kernel Team
> 
> On 11/03/2017 07:47 PM, Stanislav Kinsburskiy wrote:
>> From: Stanislav Kinsburskiy <skinsbur...@parallels.com>
>>
>> The problem is that per-net SUNRPC transports shutdown is done regardless
>> current callback execution. This is a race leading to transport 
>> use-after-free
>> in callback handler.
>> This patch fixes it in stright-forward way. I.e. it protects callback
>> execution with the same mutex used for per-net data creation and destruction.
>> Hopefully, it won't slow down NFS client significantly.
>>
>> https://jira.sw.ru/browse/PSBM-75751
>>
>> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@parallels.com>
>> ---
>>  fs/nfs/callback.c |    3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
>> index 0beb275..82e8ed1 100644
>> --- a/fs/nfs/callback.c
>> +++ b/fs/nfs/callback.c
>> @@ -118,6 +118,7 @@ nfs41_callback_svc(void *vrqstp)
>>  continue;
>>
>>  prepare_to_wait(>sv_cb_waitq, , TASK_INTERRUPTIBLE);
>> +    mutex_lock(_callback_mutex);
>>  spin_lock_bh(>sv_cb_lock);
>>  if (!list_empty(>sv_cb_list)) {
>>  req = list_first_entry(>sv_cb_list,
>> @@ -129,8 +130,10 @@ nfs41_callback_svc(void *vrqstp)
>>  error = bc_svc_process(serv, req, rqstp);
>>  dprintk("bc_svc_process() returned w/ error code= %d\n",
>>  error);
>> +    mutex_unlock(_callback_mutex);
>>  } else {
>>  spin_unlock_bh(>sv_cb_lock);
>> +    mutex_unlock(_callback_mutex);
>>  schedule();
>>  finish_wait(>sv_cb_waitq, );
>>  }
>>
>> ___
>> Devel mailing list
>> Devel@openvz.org
>> https://lists.openvz.org/mailman/listinfo/devel
>> .
>>
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] nfs: protect callback execution against per-net callback thread shutdown

[Stanislav Kinsburskiy]

From: Stanislav Kinsburskiy <skinsbur...@parallels.com>

The problem is that per-net SUNRPC transports shutdown is done regardless
current callback execution. This is a race leading to transport use-after-free
in callback handler.
This patch fixes it in stright-forward way. I.e. it protects callback
execution with the same mutex used for per-net data creation and destruction.
Hopefully, it won't slow down NFS client significantly.

https://jira.sw.ru/browse/PSBM-75751

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@parallels.com>
---
 fs/nfs/callback.c |3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index 0beb275..82e8ed1 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -118,6 +118,7 @@ nfs41_callback_svc(void *vrqstp)
continue;
 
prepare_to_wait(>sv_cb_waitq, , TASK_INTERRUPTIBLE);
+   mutex_lock(_callback_mutex);
spin_lock_bh(>sv_cb_lock);
if (!list_empty(>sv_cb_list)) {
req = list_first_entry(>sv_cb_list,
@@ -129,8 +130,10 @@ nfs41_callback_svc(void *vrqstp)
error = bc_svc_process(serv, req, rqstp);
dprintk("bc_svc_process() returned w/ error code= %d\n",
error);
+   mutex_unlock(_callback_mutex);
} else {
spin_unlock_bh(>sv_cb_lock);
+   mutex_unlock(_callback_mutex);
schedule();
finish_wait(>sv_cb_waitq, );
}

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH rh7] netfilter: Allow xt_owner in any user namespace

[Stanislav Kinsburskiy]

17.10.2017 08:53, Andrei Vagin пишет:
> On Mon, Oct 16, 2017 at 05:50:38PM +0200, Stanislav Kinsburskiy wrote:
>> Well, patch looks ok.
>> But shouldn't all the ve_init_user_ns() replaced by the par->net?
> 
> This patch does this.
> 

Yes, but not everywhere.
Say, there are owner_mt_ve0 and owner_mt6_ve0.
Shouldn't there functions also patched?

>>
>> 14.10.2017 01:20, Andrei Vagin пишет:
>>> From: "Eric W. Biederman" <ebied...@xmission.com>
>>>
>>> ML: 9847371a84b0be330f4bc4aaa98904101ee8573d
>>> https://jira.sw.ru/browse/PSBM-69409?
>>>
>>> Making this work is a little tricky as it really isn't kosher to
>>> change the xt_owner_match_info in a check function.
>>>
>>> Without changing xt_owner_match_info we need to know the user
>>> namespace the uids and gids are specified in.  In the common case
>>> net->user_ns == current_user_ns().  Verify net->user_ns ==
>>> current_user_ns() in owner_check so we can later assume it in
>>> owner_mt.
>>>
>>> In owner_check also verify that all of the uids and gids specified are
>>> in net->user_ns and that the expected min/max relationship exists
>>> between the uids and gids in xt_owner_match_info.
>>>
>>> In owner_mt get the network namespace from the outgoing socket, as this
>>> must be the same network namespace as the netfilter rules, and use that
>>> network namespace to find the user namespace the uids and gids in
>>> xt_match_owner_info are encoded in.  Then convert from their encoded
>>> from into the kernel internal format for uids and gids and perform the
>>> owner match.
>>>
>>> Similar to ping_group_range, this code does not try to detect
>>> noncontiguous UID/GID ranges.
>>>
>>> Signed-off-by: "Eric W. Biederman" <ebied...@xmission.com>
>>> Signed-off-by: Kevin Cernekee <cerne...@chromium.org>
>>> Signed-off-by: Pablo Neira Ayuso <pa...@netfilter.org>
>>> Signed-off-by: Andrei Vagin <ava...@virtuozzo.com>
>>> ---
>>>  net/netfilter/xt_owner.c | 41 +++--
>>>  1 file changed, 35 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/net/netfilter/xt_owner.c b/net/netfilter/xt_owner.c
>>> index 31dec4a..1744f78 100644
>>> --- a/net/netfilter/xt_owner.c
>>> +++ b/net/netfilter/xt_owner.c
>>> @@ -80,11 +80,39 @@ owner_mt6_v0(const struct sk_buff *skb, struct 
>>> xt_action_param *par)
>>>  static int owner_check(const struct xt_mtchk_param *par)
>>>  {
>>> struct xt_owner_match_info *info = par->matchinfo;
>>> +   struct net *net = par->net;
>>>  
>>> -   /* For now only allow adding matches from the initial user namespace */
>>> +   /* Only allow the common case where the userns of the writer
>>> +* matches the userns of the network namespace.
>>> +*/
>>> if ((info->match & (XT_OWNER_UID|XT_OWNER_GID)) &&
>>> -   !current_user_ns_initial())
>>> +   (current_user_ns() != net->user_ns))
>>> return -EINVAL;
>>> +
>>> +   /* Ensure the uids are valid */
>>> +   if (info->match & XT_OWNER_UID) {
>>> +   kuid_t uid_min = make_kuid(net->user_ns, info->uid_min);
>>> +   kuid_t uid_max = make_kuid(net->user_ns, info->uid_max);
>>> +
>>> +   if (!uid_valid(uid_min) || !uid_valid(uid_max) ||
>>> +   (info->uid_max < info->uid_min) ||
>>> +   uid_lt(uid_max, uid_min)) {
>>> +   return -EINVAL;
>>> +   }
>>> +   }
>>> +
>>> +   /* Ensure the gids are valid */
>>> +   if (info->match & XT_OWNER_GID) {
>>> +   kgid_t gid_min = make_kgid(net->user_ns, info->gid_min);
>>> +   kgid_t gid_max = make_kgid(net->user_ns, info->gid_max);
>>> +
>>> +   if (!gid_valid(gid_min) || !gid_valid(gid_max) ||
>>> +   (info->gid_max < info->gid_min) ||
>>> +   gid_lt(gid_max, gid_min)) {
>>> +   return -EINVAL;
>>> +   }
>>> +   }
>>> +
>>> return 0;
>>>  }
>>>  
>>> @@ -93,6 +121,7 @@ owner_mt(const struct sk_buff *skb, struct 
>>> xt_action_param *par)
>>>  {
>>> const struct xt_owner_match_info *info = par->matchinfo;
>

[Devel] [PATCH] venet: destroy VE IP on venet destruction in NFS is enabled

[Stanislav Kinsburskiy]

We skip VE IP destruion in shutdown hook, if NFS is enabled in CT (to allow
NFS mounts to disappear.
Thus we have to destroy it with venet device.

https://jira.sw.ru/browse/PSBM-75120

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 drivers/net/venetdev.c |7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/venetdev.c b/drivers/net/venetdev.c
index 7a546cc..fad232b 100644
--- a/drivers/net/venetdev.c
+++ b/drivers/net/venetdev.c
@@ -759,9 +759,12 @@ static void venet_dellink(struct net_device *dev, struct 
list_head *head)
struct ve_struct *env = dev->nd_net->owner_ve;
 
/* We check ve_netns to avoid races with veip SHUTDOWN hook, called from
-* ve_exit_ns()
+* ve_exit_ns().
+* Also, in veip SHUTDOWN hook we skip veip destructionif, if container
+* has VE_FEATURE_NFS enabled. Thus here we have to destroy veip in
+* this case.
 */
-   if (env->ve_netns)
+   if (env->ve_netns || (env->features & VE_FEATURE_NFS))
veip_shutdown(env);
 
env->_venet_dev = NULL;

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] venet: do not destroy VE IP on shutdown hook if NFS is allowed

[Stanislav Kinsburskiy]

Mounts are destroyed asynchroniously and thus can race with VE IP destruction
leading to dead lock in kernel: NFS can'n be unmounted.
Fix it by skipping VE IP desctruction, is NFS feature is enabled in the
container.

https://jira.sw.ru/browse/PSBM-73614

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 drivers/net/venetdev.c |   16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/venetdev.c b/drivers/net/venetdev.c
index 5710792..7a546cc 100644
--- a/drivers/net/venetdev.c
+++ b/drivers/net/venetdev.c
@@ -744,10 +744,8 @@ static void venet_setup(struct net_device *dev)
SET_ETHTOOL_OPS(dev, _ethtool_ops);
 }
 
-static void veip_shutdown(void *data)
+static void veip_shutdown(struct ve_struct *ve)
 {
-   struct ve_struct *ve = data;
-
spin_lock(_lock);
if (ve->veip) {
__venet_ext_clean(ve);
@@ -1178,8 +1176,18 @@ static struct rtnl_link_ops venet_link_ops = {
.maxtype= VENET_INFO_MAX,
 };
 
+static void veip_shutdown_fini(void *data)
+{
+   struct ve_struct *ve = data;
+
+   if (ve->features & VE_FEATURE_NFS)
+   return;
+
+   veip_shutdown(ve);
+}
+
 static struct ve_hook veip_shutdown_hook = {
-   .fini   = veip_shutdown,
+   .fini   = veip_shutdown_fini,
.priority   = HOOK_PRIO_FINISHING,
.owner  = THIS_MODULE,
 };

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] ve: don't call shutdown hook for now

[Stanislav Kinsburskiy]

It was me.
The reason is explained in the patch description (you can find more details in 
description of the commit, introduced the hook).
I'm waiting for khorenko@ to convince him, that the idea to "podkovat' blohy" 
was wrong by design and we should drop the hook.
Once I succeed, I'll sent appropriate series to get rid of it completely.
So this patch is temporary one, because too many tests have failed because of 
this race.


2 окт. 2017 г. 8:05 PM пользователь Andrey Vagin <ava...@virtuozzo.com> написал:
On Mon, Oct 02, 2017 at 06:47:19PM +0400, Stanislav Kinsburskiy wrote:
> This hook is needed only for releaseing venet IP address early (thus allowing
> to restart container with the same IP faster).
> But mount are destroyed asynchroniosly, and thus NFS mount can be destroyed
> after IP address is dropped.
> Let's fir this race it the way how all the world does things: release IP with
> network namespace.

Why don't you remove this code? Who added this hook? What was a reason?

>
> https://jira.sw.ru/browse/PSBM-73193
>
> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
> ---
>  kernel/ve/ve.c |3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/ve/ve.c b/kernel/ve/ve.c
> index b0188c3..4efb9d9 100644
> --- a/kernel/ve/ve.c
> +++ b/kernel/ve/ve.c
> @@ -451,7 +451,8 @@ static void ve_drop_context(struct ve_struct *ve)
>synchronize_rcu();
>put_nsproxy(ve_ns);
>
> - ve_hook_iterate_fini(VE_SHUTDOWN_CHAIN, ve);
> + /* This have to be revisited */
> +//   ve_hook_iterate_fini(VE_SHUTDOWN_CHAIN, ve);
>
>put_cred(ve->init_cred);
>ve->init_cred = NULL;
>
> ___
> Devel mailing list
> Devel@openvz.org
> https://lists.openvz.org/mailman/listinfo/devel

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] ve: don't call shutdown hook for now

[Stanislav Kinsburskiy]

This hook is needed only for releaseing venet IP address early (thus allowing
to restart container with the same IP faster).
But mount are destroyed asynchroniosly, and thus NFS mount can be destroyed
after IP address is dropped.
Let's fir this race it the way how all the world does things: release IP with
network namespace.

https://jira.sw.ru/browse/PSBM-73193

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 kernel/ve/ve.c |3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/ve/ve.c b/kernel/ve/ve.c
index b0188c3..4efb9d9 100644
--- a/kernel/ve/ve.c
+++ b/kernel/ve/ve.c
@@ -451,7 +451,8 @@ static void ve_drop_context(struct ve_struct *ve)
synchronize_rcu();
put_nsproxy(ve_ns);
 
-   ve_hook_iterate_fini(VE_SHUTDOWN_CHAIN, ve);
+   /* This have to be revisited */
+// ve_hook_iterate_fini(VE_SHUTDOWN_CHAIN, ve);
 
put_cred(ve->init_cred);
ve->init_cred = NULL;

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] scripts: add "-w" to iptables command

[Stanislav Kinsburskiy]

How old should it be?
I checked with v1.4.21

28.09.2017 12:55, Kirill Tkhai пишет:
> Could you please to say will it work on old iptables?
> 
> On 28.09.2017 13:03, Stanislav Kinsburskiy wrote:
>> What a brilliant idea it was to ignore unknown keys.
>> Should take it into account.
>>
>> 28.09.2017 10:26, Vasily Averin пишет:
>>> kthai@ explained that old version of iptables ignores unknown keys, so 
>>> adding -w is safe.
>>>
>>> On 2017-09-28 10:40, Pavel Tikhomirov wrote:
>>>> Can we have these script running with older iptables version which does 
>>>> not have "-w"?
>>>>
>>>> On 09/27/2017 02:11 PM, Stanislav Kinsburskiy wrote:
>>>>> Neede to support new versions of iptables.
>>>>>
>>>>> https://jira.sw.ru/browse/PSBM-73153
>>>>>
>>>>> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
>>>>> ---
>>>>>   scripts/nfs-ports-allow.sh |   16 
>>>>>   1 file changed, 8 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/scripts/nfs-ports-allow.sh b/scripts/nfs-ports-allow.sh
>>>>> index 97541dc..ac5cf5f 100644
>>>>> --- a/scripts/nfs-ports-allow.sh
>>>>> +++ b/scripts/nfs-ports-allow.sh
>>>>> @@ -36,10 +36,10 @@ function add_accept_rules {
>>>>>   local server=$1
>>>>>   local port=$2
>>>>>   -${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -s 
>>>>> $server --sport $port -j ACCEPT &&
>>>>> -${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -d 
>>>>> $server --dport $port -j ACCEPT &&
>>>>> -${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p udp -s 
>>>>> $server --sport $port -j ACCEPT &&
>>>>> -${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p udp -d 
>>>>> $server --dport $port -j ACCEPT
>>>>> +${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -s 
>>>>> $server --sport $port -j ACCEPT &&
>>>>> +${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -d 
>>>>> $server --dport $port -j ACCEPT &&
>>>>> +${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p udp -s 
>>>>> $server --sport $port -j ACCEPT &&
>>>>> +${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p udp -d 
>>>>> $server --dport $port -j ACCEPT
>>>>>   }
>>>>> function iptables_allow_nfs_ports {
>>>>> @@ -63,10 +63,10 @@ function allow_portmapper_port {
>>>>>   local server=$1
>>>>>   local port=111
>>>>>   -${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p udp -s 
>>>>> $server --sport $port -j ACCEPT &&
>>>>> -${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p udp -d 
>>>>> $server --dport $port -j ACCEPT &&
>>>>> -${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -s 
>>>>> $server --sport $port -j ACCEPT &&
>>>>> -${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -d 
>>>>> $server --dport $port -j ACCEPT
>>>>> +${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p udp -s 
>>>>> $server --sport $port -j ACCEPT &&
>>>>> +${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p udp -d 
>>>>> $server --dport $port -j ACCEPT &&
>>>>> +${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -s 
>>>>> $server --sport $port -j ACCEPT &&
>>>>> +${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -d 
>>>>> $server --dport $port -j ACCEPT
>>>>>   }
>>>>> for s in $servers; do
>>>>>
>>>>> ___
>>>>> Devel mailing list
>>>>> Devel@openvz.org
>>>>> https://lists.openvz.org/mailman/listinfo/devel
>>>>>
>>>>
>>> ___
>>> Devel mailing list
>>> Devel@openvz.org
>>> https://lists.openvz.org/mailman/listinfo/devel
>>>
>> ___
>> Devel mailing list
>> Devel@openvz.org
>> https://lists.openvz.org/mailman/listinfo/devel
>>
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] scripts: add "-w" to iptables command

[Stanislav Kinsburskiy]

What a brilliant idea it was to ignore unknown keys.
Should take it into account.

28.09.2017 10:26, Vasily Averin пишет:
> kthai@ explained that old version of iptables ignores unknown keys, so adding 
> -w is safe.
> 
> On 2017-09-28 10:40, Pavel Tikhomirov wrote:
>> Can we have these script running with older iptables version which does not 
>> have "-w"?
>>
>> On 09/27/2017 02:11 PM, Stanislav Kinsburskiy wrote:
>>> Neede to support new versions of iptables.
>>>
>>> https://jira.sw.ru/browse/PSBM-73153
>>>
>>> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
>>> ---
>>>   scripts/nfs-ports-allow.sh |   16 
>>>   1 file changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/scripts/nfs-ports-allow.sh b/scripts/nfs-ports-allow.sh
>>> index 97541dc..ac5cf5f 100644
>>> --- a/scripts/nfs-ports-allow.sh
>>> +++ b/scripts/nfs-ports-allow.sh
>>> @@ -36,10 +36,10 @@ function add_accept_rules {
>>>   local server=$1
>>>   local port=$2
>>>   -${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -s 
>>> $server --sport $port -j ACCEPT &&
>>> -${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -d $server 
>>> --dport $port -j ACCEPT &&
>>> -${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p udp -s $server 
>>> --sport $port -j ACCEPT &&
>>> -${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p udp -d $server 
>>> --dport $port -j ACCEPT
>>> +${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -s 
>>> $server --sport $port -j ACCEPT &&
>>> +${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -d 
>>> $server --dport $port -j ACCEPT &&
>>> +${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p udp -s 
>>> $server --sport $port -j ACCEPT &&
>>> +${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p udp -d 
>>> $server --dport $port -j ACCEPT
>>>   }
>>> function iptables_allow_nfs_ports {
>>> @@ -63,10 +63,10 @@ function allow_portmapper_port {
>>>   local server=$1
>>>   local port=111
>>>   -${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p udp -s 
>>> $server --sport $port -j ACCEPT &&
>>> -${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p udp -d $server 
>>> --dport $port -j ACCEPT &&
>>> -${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -s $server 
>>> --sport $port -j ACCEPT &&
>>> -${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -d $server 
>>> --dport $port -j ACCEPT
>>> +${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p udp -s 
>>> $server --sport $port -j ACCEPT &&
>>> +${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p udp -d 
>>> $server --dport $port -j ACCEPT &&
>>> +${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -s 
>>> $server --sport $port -j ACCEPT &&
>>> +${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -d 
>>> $server --dport $port -j ACCEPT
>>>   }
>>> for s in $servers; do
>>>
>>> ___
>>> Devel mailing list
>>> Devel@openvz.org
>>> https://lists.openvz.org/mailman/listinfo/devel
>>>
>>
> ___
> Devel mailing list
> Devel@openvz.org
> https://lists.openvz.org/mailman/listinfo/devel
> 
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] scripts: add "-w" to iptables command

[Stanislav Kinsburskiy]

Neede to support new versions of iptables.

https://jira.sw.ru/browse/PSBM-73153

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 scripts/nfs-ports-allow.sh |   16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/scripts/nfs-ports-allow.sh b/scripts/nfs-ports-allow.sh
index 97541dc..ac5cf5f 100644
--- a/scripts/nfs-ports-allow.sh
+++ b/scripts/nfs-ports-allow.sh
@@ -36,10 +36,10 @@ function add_accept_rules {
local server=$1
local port=$2
 
-   ${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -s $server 
--sport $port -j ACCEPT &&
-   ${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -d $server 
--dport $port -j ACCEPT &&
-   ${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p udp -s $server 
--sport $port -j ACCEPT &&
-   ${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p udp -d $server 
--dport $port -j ACCEPT 
+   ${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -s 
$server --sport $port -j ACCEPT &&
+   ${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -d 
$server --dport $port -j ACCEPT &&
+   ${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p udp -s 
$server --sport $port -j ACCEPT &&
+   ${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p udp -d 
$server --dport $port -j ACCEPT 
 }
 
 function iptables_allow_nfs_ports {
@@ -63,10 +63,10 @@ function allow_portmapper_port {
local server=$1
local port=111
 
-   ${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p udp -s $server 
--sport $port -j ACCEPT &&
-   ${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p udp -d $server 
--dport $port -j ACCEPT &&
-   ${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -s $server 
--sport $port -j ACCEPT &&
-   ${JOIN_CT} ${IPTABLES} -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -d $server 
--dport $port -j ACCEPT 
+   ${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p udp -s 
$server --sport $port -j ACCEPT &&
+   ${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p udp -d 
$server --dport $port -j ACCEPT &&
+   ${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -s 
$server --sport $port -j ACCEPT &&
+   ${JOIN_CT} ${IPTABLES} -w -I ${CRTOOLS_IPTABLES_TABLE} -p tcp -d 
$server --dport $port -j ACCEPT 
 }
 
 for s in $servers; do

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] connector: bump skb->users before callback invocation

[Stanislav Kinsburskiy]

From: Florian Westphal <f...@strlen.de>

Backport of commit 55285bf09427c5abf43ee1d54e892f352092b1f1.

Dmitry reports memleak with syskaller program.
Problem is that connector bumps skb usecount but might not invoke callback.

So move skb_get to where we invoke the callback.

https://jira.sw.ru/browse/PSBM-71904

Reported-by: Dmitry Vyukov <dvyu...@google.com>
Signed-off-by: Florian Westphal <f...@strlen.de>
Signed-off-by: David S. Miller <da...@davemloft.net>
Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 drivers/connector/connector.c |   11 +++
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
index 752c692..57285ba 100644
--- a/drivers/connector/connector.c
+++ b/drivers/connector/connector.c
@@ -161,26 +161,21 @@ static int cn_call_callback(struct sk_buff *skb)
  *
  * It checks skb, netlink header and msg sizes, and calls callback helper.
  */
-static void cn_rx_skb(struct sk_buff *__skb)
+static void cn_rx_skb(struct sk_buff *skb)
 {
struct nlmsghdr *nlh;
-   struct sk_buff *skb;
int len, err;
 
-   skb = skb_get(__skb);
-
if (skb->len >= NLMSG_HDRLEN) {
nlh = nlmsg_hdr(skb);
len = nlmsg_len(nlh);
 
if (len < (int)sizeof(struct cn_msg) ||
skb->len < nlh->nlmsg_len ||
-   len > CONNECTOR_MAX_MSG_SIZE) {
-   kfree_skb(skb);
+   len > CONNECTOR_MAX_MSG_SIZE)
return;
-   }
 
-   err = cn_call_callback(skb);
+   err = cn_call_callback(skb_get(skb));
if (err < 0)
kfree_skb(skb);
}

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation

[Stanislav Kinsburskiy]

14.09.2017 13:45, Ian Kent пишет:
> On 14/09/17 19:39, Stanislav Kinsburskiy wrote:
>>
>>
>> 14.09.2017 13:29, Ian Kent пишет:
>>> On 14/09/17 17:24, Stanislav Kinsburskiy wrote:
>>>>
>>>>
>>>> 14.09.2017 02:38, Ian Kent пишет:
>>>>> On 01/09/17 19:21, Stanislav Kinsburskiy wrote:
>>>>>> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
>>>>>> ---
>>>>>>  fs/autofs4/autofs_i.h  |3 +++
>>>>>>  fs/autofs4/dev-ioctl.c |3 +++
>>>>>>  fs/autofs4/inode.c |4 +++-
>>>>>>  3 files changed, 9 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
>>>>>> index 4737615..3da105f 100644
>>>>>> --- a/fs/autofs4/autofs_i.h
>>>>>> +++ b/fs/autofs4/autofs_i.h
>>>>>> @@ -120,6 +120,9 @@ struct autofs_sb_info {
>>>>>>  struct list_head active_list;
>>>>>>  struct list_head expiring_list;
>>>>>>  struct rcu_head rcu;
>>>>>> +#ifdef CONFIG_COMPAT
>>>>>> +unsigned is32bit:1;
>>>>>> +#endif
>>>>>>  };
>>>>>>  
>>>>>>  static inline struct autofs_sb_info *autofs4_sbi(struct super_block *sb)
>>>>>> diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
>>>>>> index b7c816f..467d6c4 100644
>>>>>> --- a/fs/autofs4/dev-ioctl.c
>>>>>> +++ b/fs/autofs4/dev-ioctl.c
>>>>>> @@ -397,6 +397,9 @@ static int autofs_dev_ioctl_setpipefd(struct file 
>>>>>> *fp,
>>>>>>  sbi->pipefd = pipefd;
>>>>>>  sbi->pipe = pipe;
>>>>>>  sbi->catatonic = 0;
>>>>>> +#ifdef CONFIG_COMPAT
>>>>>> +sbi->is32bit = is_compat_task();
>>>>>> +#endif
>>>>>>  }
>>>>>>  out:
>>>>>>  put_pid(new_pid);
>>>>>> diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
>>>>>> index 09e7d68..21d3c0b 100644
>>>>>> --- a/fs/autofs4/inode.c
>>>>>> +++ b/fs/autofs4/inode.c
>>>>>> @@ -301,7 +301,9 @@ int autofs4_fill_super(struct super_block *s, void 
>>>>>> *data, int silent)
>>>>>>  } else {
>>>>>>  sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
>>>>>>  }
>>>>>> -
>>>>>> +#ifdef CONFIG_COMPAT
>>>>>> +sbi->is32bit = is_compat_task();
>>>>>> +#endif
>>>>>>  if (autofs_type_trigger(sbi->type))
>>>>>>  __managed_dentry_set_managed(root);
>>>>>>  
>>>>>>
>>>>>
>>>>> Not sure about this.
>>>>>
>>>>> Don't you think it would be better to avoid the in code #ifdefs by doing 
>>>>> some
>>>>> checks and defines in the header file and defining what's need to just use
>>>>> is_compat_task().
>>>>>
>>>>
>>>> Yes, might be...
>>>>
>>>>> Not sure 2 patches are needed for this either ..
>>>>>
>>>>
>>>> Well, I found this issue occasionally.
>>>
>>> I'm wondering what the symptoms are?
>>>
>>
>> Size of struct autofs_v5_packet is 300 bytes for x86 and 304 bytes for 
>> x86_64.
>> Which means, that 32bit task can read more than size of autofs_v5_packet on 
>> 64bit kernel.
> 
> Are you sure?
> 
> Shouldn't that be a short read on the x86 side of a 4 bytes longer
> structure on the x86_64 side.
> 
> I didn't think you could have a 64 bit client on a 32 bit kernel
> so the converse (the read past end of struct) doesn't apply.
> 

Sorry for the confusion, I had to add brackets like this:

> Which means, that 32bit task can read more than size of autofs_v5_packet (on 
> 64bit kernel).

IOW, 32bit task expects to read 300 bytes (size of struct autofs_v5_packet) 
while there are 304 bytes available on the "wire" from the 64bit kernel.


> Ian
> 
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation

[Stanislav Kinsburskiy]

14.09.2017 13:29, Ian Kent пишет:
> On 14/09/17 17:24, Stanislav Kinsburskiy wrote:
>>
>>
>> 14.09.2017 02:38, Ian Kent пишет:
>>> On 01/09/17 19:21, Stanislav Kinsburskiy wrote:
>>>> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
>>>> ---
>>>>  fs/autofs4/autofs_i.h  |3 +++
>>>>  fs/autofs4/dev-ioctl.c |3 +++
>>>>  fs/autofs4/inode.c |4 +++-
>>>>  3 files changed, 9 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
>>>> index 4737615..3da105f 100644
>>>> --- a/fs/autofs4/autofs_i.h
>>>> +++ b/fs/autofs4/autofs_i.h
>>>> @@ -120,6 +120,9 @@ struct autofs_sb_info {
>>>>struct list_head active_list;
>>>>struct list_head expiring_list;
>>>>struct rcu_head rcu;
>>>> +#ifdef CONFIG_COMPAT
>>>> +  unsigned is32bit:1;
>>>> +#endif
>>>>  };
>>>>  
>>>>  static inline struct autofs_sb_info *autofs4_sbi(struct super_block *sb)
>>>> diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
>>>> index b7c816f..467d6c4 100644
>>>> --- a/fs/autofs4/dev-ioctl.c
>>>> +++ b/fs/autofs4/dev-ioctl.c
>>>> @@ -397,6 +397,9 @@ static int autofs_dev_ioctl_setpipefd(struct file *fp,
>>>>sbi->pipefd = pipefd;
>>>>sbi->pipe = pipe;
>>>>sbi->catatonic = 0;
>>>> +#ifdef CONFIG_COMPAT
>>>> +  sbi->is32bit = is_compat_task();
>>>> +#endif
>>>>}
>>>>  out:
>>>>put_pid(new_pid);
>>>> diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
>>>> index 09e7d68..21d3c0b 100644
>>>> --- a/fs/autofs4/inode.c
>>>> +++ b/fs/autofs4/inode.c
>>>> @@ -301,7 +301,9 @@ int autofs4_fill_super(struct super_block *s, void 
>>>> *data, int silent)
>>>>} else {
>>>>sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
>>>>}
>>>> -
>>>> +#ifdef CONFIG_COMPAT
>>>> +  sbi->is32bit = is_compat_task();
>>>> +#endif
>>>>if (autofs_type_trigger(sbi->type))
>>>>__managed_dentry_set_managed(root);
>>>>  
>>>>
>>>
>>> Not sure about this.
>>>
>>> Don't you think it would be better to avoid the in code #ifdefs by doing 
>>> some
>>> checks and defines in the header file and defining what's need to just use
>>> is_compat_task().
>>>
>>
>> Yes, might be...
>>
>>> Not sure 2 patches are needed for this either ..
>>>
>>
>> Well, I found this issue occasionally.
> 
> I'm wondering what the symptoms are?
> 

Size of struct autofs_v5_packet is 300 bytes for x86 and 304 bytes for x86_64.
Which means, that 32bit task can read more than size of autofs_v5_packet on 
64bit kernel.

>> And, frankly speaking, it's not clear to me, whether this issue is important 
>> at all, so I wanted to clarify this first.
>> Thanks to O_DIRECT, the only way to catch the issue is to try to read more, 
>> than expected, in compat task (that's how I found it).
> 
> Right, the O_DIRECT patch from Linus was expected to fix the structure
> alignment problem. The stuct field offsets are ok aren't they?
> 

Yes, they are ok.

>> I don't see any other flaw so far. And if so, that, probably, we shouldn't 
>> care about the issue at all.
>> What do you think?
> 
> If we are seeing hangs, incorrect struct fields or similar something
> should be done about it but if all is actually working ok then the
> O_DIRECT fix is doing it's job and further changes aren't necessary.
> 

Well, yes. O_DIRECT fix covers the issue.
Ok then.
Thanks for the clarification!

> Ian
> 
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation

[Stanislav Kinsburskiy]

14.09.2017 02:38, Ian Kent пишет:
> On 01/09/17 19:21, Stanislav Kinsburskiy wrote:
>> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
>> ---
>>  fs/autofs4/autofs_i.h  |3 +++
>>  fs/autofs4/dev-ioctl.c |3 +++
>>  fs/autofs4/inode.c |4 +++-
>>  3 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
>> index 4737615..3da105f 100644
>> --- a/fs/autofs4/autofs_i.h
>> +++ b/fs/autofs4/autofs_i.h
>> @@ -120,6 +120,9 @@ struct autofs_sb_info {
>>  struct list_head active_list;
>>  struct list_head expiring_list;
>>  struct rcu_head rcu;
>> +#ifdef CONFIG_COMPAT
>> +unsigned is32bit:1;
>> +#endif
>>  };
>>  
>>  static inline struct autofs_sb_info *autofs4_sbi(struct super_block *sb)
>> diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
>> index b7c816f..467d6c4 100644
>> --- a/fs/autofs4/dev-ioctl.c
>> +++ b/fs/autofs4/dev-ioctl.c
>> @@ -397,6 +397,9 @@ static int autofs_dev_ioctl_setpipefd(struct file *fp,
>>  sbi->pipefd = pipefd;
>>  sbi->pipe = pipe;
>>  sbi->catatonic = 0;
>> +#ifdef CONFIG_COMPAT
>> +sbi->is32bit = is_compat_task();
>> +#endif
>>  }
>>  out:
>>  put_pid(new_pid);
>> diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
>> index 09e7d68..21d3c0b 100644
>> --- a/fs/autofs4/inode.c
>> +++ b/fs/autofs4/inode.c
>> @@ -301,7 +301,9 @@ int autofs4_fill_super(struct super_block *s, void 
>> *data, int silent)
>>  } else {
>>  sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
>>  }
>> -
>> +#ifdef CONFIG_COMPAT
>> +sbi->is32bit = is_compat_task();
>> +#endif
>>  if (autofs_type_trigger(sbi->type))
>>  __managed_dentry_set_managed(root);
>>  
>>
> 
> Not sure about this.
> 
> Don't you think it would be better to avoid the in code #ifdefs by doing some
> checks and defines in the header file and defining what's need to just use
> is_compat_task().
> 

Yes, might be...

> Not sure 2 patches are needed for this either ..
> 

Well, I found this issue occasionally.
And, frankly speaking, it's not clear to me, whether this issue is important at 
all, so I wanted to clarify this first.
Thanks to O_DIRECT, the only way to catch the issue is to try to read more, 
than expected, in compat task (that's how I found it).
I don't see any other flaw so far. And if so, that, probably, we shouldn't care 
about the issue at all.
What do you think?


> Ian
> 


 
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] sysfs: don't use VE namespace for "dev/char" directory

[Stanislav Kinsburskiy]

Looks like we don't have devices, using it. Or lost some patches.
Anyway, currently it doesn't work. To make this VE namespace work, "namespace"
callback has to be added to ve_kobj_type. And this callback has to get VE
structure from kobject somehow. IOW, device has to be allocated per-ve.
Another issue here is that it's unclear, how to mark generic devices as
allocated in VE#0 thus making them visible in VE#0.
Looks like the whole idea was wrong...

https://jira.sw.ru/browse/PSBM-71811

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 drivers/base/core.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 80cf3eb..86540d9 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1538,7 +1538,7 @@ int __init devices_init(void)
sysfs_dev_block_kobj = kobject_create_and_add("block", dev_kobj);
if (!sysfs_dev_block_kobj)
goto block_kobj_err;
-   sysfs_dev_char_kobj = kobject_create_and_add_ve("char", dev_kobj);
+   sysfs_dev_char_kobj = kobject_create_and_add("char", dev_kobj);
if (!sysfs_dev_char_kobj)
goto char_kobj_err;
 

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] zdtm: fix package memory allocation in autofs.c

[Stanislav Kinsburskiy]

No, we don't.
But this one could be applied: "tests: do not try to read more than packet in 
AutoFS test"

09.09.2017 02:01, Andrei Vagin пишет:
> On Thu, Aug 31, 2017 at 12:10:40PM +0300, Stanislav Kinsburskiy wrote:
>> Plus some cleanup.
> 
> Do we need this patch for the upstream criu? Could you send it to
> the criu mailing list?
> 
>>
>> https://jira.sw.ru/browse/PSBM-71078
>>
>> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
>> ---
>>  test/zdtm/static/autofs.c |   18 +-
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/test/zdtm/static/autofs.c b/test/zdtm/static/autofs.c
>> index 8d917ee..882289f 100644
>> --- a/test/zdtm/static/autofs.c
>> +++ b/test/zdtm/static/autofs.c
>> @@ -460,10 +460,10 @@ static int automountd_loop(int pipe, const char 
>> *mountpoint, struct autofs_param
>>  {
>>  union autofs_v5_packet_union *packet;
>>  ssize_t bytes;
>> -size_t psize = sizeof(*packet) * 2;
>> +size_t psize = sizeof(*packet);
>>  int err = 0;
>>  
>> -packet = malloc(psize);
>> +packet = malloc(psize * 2);
>>  if (!packet) {
>>  pr_err("failed to allocate autofs packet\n");
>>  return -ENOMEM;
>> @@ -473,7 +473,7 @@ static int automountd_loop(int pipe, const char 
>> *mountpoint, struct autofs_param
>>  siginterrupt(SIGUSR2, 1);
>>  
>>  while (!stop && !err) {
>> -memset(packet, 0, sizeof(*packet));
>> +memset(packet, 0, psize * 2);
>>  
>>  bytes = read(pipe, packet, psize);
>>  if (bytes < 0) {
>> @@ -483,12 +483,12 @@ static int automountd_loop(int pipe, const char 
>> *mountpoint, struct autofs_param
>>  }
>>  continue;
>>  }
>> -if (bytes > psize) {
>> -pr_err("read more that expected: %zd > %zd\n", bytes, 
>> psize);
>> -return -EINVAL;
>> -}
>> -if (bytes != sizeof(*packet)) {
>> -pr_err("read less than expected: %zd\n", bytes);
>> +if (bytes != psize) {
>> +pr_err("read %s that expected: %zd %s %zd\n",
>> +(bytes > psize) ? "more" : "less",
>> +bytes,
>> +(bytes > psize) ? ">" : "<",
>> +psize);
>>  return -EINVAL;
>>  }
>>  err = automountd_serve(mountpoint, param, packet);
>>
>> ___
>> Devel mailing list
>> Devel@openvz.org
>> https://lists.openvz.org/mailman/listinfo/devel
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] tcp: fix 0 divide in __tcp_select_window()

[Stanislav Kinsburskiy]

From: Eric Dumazet <eduma...@google.com>

syszkaller fuzzer was able to trigger a divide by zero, when
TCP window scaling is not enabled.

SO_RCVBUF can be used not only to increase sk_rcvbuf, also
to decrease it below current receive buffers utilization.

If mss is negative or 0, just return a zero TCP window.

https://jira.sw.ru/browse/PSBM-71285

Back-port of ML commit: 06425c308b92eaf60767bc71d359f4cbc7a561f8

Signed-off-by: Eric Dumazet <eduma...@google.com>
Reported-by: Dmitry Vyukov  <dvyu...@google.com>
Acked-by: Neal Cardwell <ncardw...@google.com>
Signed-off-by: David S. Miller <da...@davemloft.net>
Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 net/ipv4/tcp_output.c |6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index f395a09..3d24481 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2295,9 +2295,11 @@ u32 __tcp_select_window(struct sock *sk)
int full_space = min_t(int, tp->window_clamp, allowed_space);
int window;
 
-   if (mss > full_space)
+   if (unlikely(mss > full_space)) {
mss = full_space;
-
+   if (mss <= 0)
+   return 0;
+   }
if (free_space < (full_space >> 1)) {
icsk->icsk_ack.quick = 0;
 

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [RFC PATCH 2/2] autofs: sent 32-bit sized packet for 32-bit process

[Stanislav Kinsburskiy]

03.09.2017 21:23, Dmitry V. Levin P?P8QP5Q:
> On Sat, Sep 02, 2017 at 02:26:26PM +0300, Stanislav Kinsburskiy wrote:
>> 01.09.2017 21:23, Dmitry V. Levin P?P8QP5Q:
>>> On Fri, Sep 01, 2017 at 05:02:45PM +0300, Stanislav Kinsburskiy wrote:
>>>> 01.09.2017 16:53, Dmitry V. Levin P?P8QP5Q:
>>>>> On Fri, Sep 01, 2017 at 12:15:17PM +0300, Stanislav Kinsburskiy wrote:
>>>>>> 31.08.2017 20:22, Dmitry V. Levin P?P8QP5Q:
>>>>>>> On Thu, Aug 31, 2017 at 05:57:11PM +0400, Stanislav Kinsburskiy wrote:
>>>>>>>> The structure autofs_v5_packet (except name) is not aligned by 8 
>>>>>>>> bytes, which
>>>>>>>> lead to different sizes in 32 and 64-bit architectures.
>>>>>>>> Let's form 32-bit compatible packet when daemon has 32-bit 
>>>>>>>> addressation.
>>>>>>>>
>>>>>>>> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
>>>>>>>> ---
>>>>>>>>  fs/autofs4/waitq.c |   11 +--
>>>>>>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
>>>>>>>> index 309ca6b..484cf2e 100644
>>>>>>>> --- a/fs/autofs4/waitq.c
>>>>>>>> +++ b/fs/autofs4/waitq.c
>>>>>>>> @@ -153,12 +153,19 @@ static void autofs4_notify_daemon(struct 
>>>>>>>> autofs_sb_info *sbi,
>>>>>>>>{
>>>>>>>>struct autofs_v5_packet *packet = _pkt.v5_packet;
>>>>>>>>struct user_namespace *user_ns = 
>>>>>>>> sbi->pipe->f_cred->user_ns;
>>>>>>>> +  size_t name_offset;
>>>>>>>>  
>>>>>>>> -  pktsz = sizeof(*packet);
>>>>>>>> +  if (sbi->is32bit)
>>>>>>>> +  name_offset = offsetof(struct autofs_v5_packet, 
>>>>>>>> len) +
>>>>>>>> +sizeof(packet->len);
>>>>>>>> +  else
>>>>>>>> +  name_offset = offsetof(struct autofs_v5_packet, 
>>>>>>>> name);
>>>>>>>
>>>>>>> This doesn't help at all because the offset of struct 
>>>>>>> autofs_v5_packet.name
>>>>>>> does not change.
>>>>>>>
>>>>>>>> +  pktsz = name_offset + sizeof(packet->name);
>>>>>>>
>>>>>>> What changes is pktsz: it's either sizeof(struct autofs_v5_packet)
>>>>>>> or 4 bytes less, depending on the architecture.
>>>>>>
>>>>>> Indeed. Thanks!
>>>>>>
>>>>>>> For example,
>>>>>>>
>>>>>>> #ifdef CONFIG_COMPAT
>>>>>>> if (__alignof__(compat_u64) < __alignof__(u64) && sbi->is32bit)
>>>>>>
>>>>>> Unfortunately I don't get this "alignof" checks.
>>>>>> The intention of "is32bit" was to define this compat case completely.
>>>>>> Why are they required?
>>>>>
>>>>> On some 32-bit architectures like arm, u64 is 64-bit aligned, on others
>>>>> like x86 it is not.  This alignof check ensures that compat 32-bit
>>>>> architectures with 64-bit alignment are not going to be broken
>>>>> by the change.
>>>>
>>>> Thanks for the explanation!
>>>> But looks like the issue is hidden so deep (thanks to O_DIRECT), that 
>>>> becomes unimportant.
>>>
>>> I'm not quite sure how O_DIRECT makes this unimportant.
>>> For example, automount_dispatch_io from systemd tries to read exactly
>>> sizeof(union autofs_v5_packet_union) bytes and fails in case of short read.
>>
>> That's weird. Can you reproduce it?
>> The problem is that kernel always send arch packet (i.e. 300 bytes for x86, 
>> or 304 for x86_64).
> 
> Well, if sending a lengthy packet is not a problem, what is the problem
> you were trying to solve by patching struct autofs_v5_packet?
> 

Well, nothing anymore. I took some time to realize, that this issue doesn't 
hurt anyone.
So I fixed our test instead.

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [RFC PATCH 2/2] autofs: sent 32-bit sized packet for 32-bit process

[Stanislav Kinsburskiy]

01.09.2017 21:23, Dmitry V. Levin P?P8QP5Q:
> On Fri, Sep 01, 2017 at 05:02:45PM +0300, Stanislav Kinsburskiy wrote:
>>
>>
>> 01.09.2017 16:53, Dmitry V. Levin P?P8QP5Q:
>>> On Fri, Sep 01, 2017 at 12:15:17PM +0300, Stanislav Kinsburskiy wrote:
>>>> 31.08.2017 20:22, Dmitry V. Levin P?P8QP5Q:
>>>>> On Thu, Aug 31, 2017 at 05:57:11PM +0400, Stanislav Kinsburskiy wrote:
>>>>>> The structure autofs_v5_packet (except name) is not aligned by 8 bytes, 
>>>>>> which
>>>>>> lead to different sizes in 32 and 64-bit architectures.
>>>>>> Let's form 32-bit compatible packet when daemon has 32-bit addressation.
>>>>>>
>>>>>> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
>>>>>> ---
>>>>>>  fs/autofs4/waitq.c |   11 +--
>>>>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
>>>>>> index 309ca6b..484cf2e 100644
>>>>>> --- a/fs/autofs4/waitq.c
>>>>>> +++ b/fs/autofs4/waitq.c
>>>>>> @@ -153,12 +153,19 @@ static void autofs4_notify_daemon(struct 
>>>>>> autofs_sb_info *sbi,
>>>>>>  {
>>>>>>  struct autofs_v5_packet *packet = _pkt.v5_packet;
>>>>>>  struct user_namespace *user_ns = 
>>>>>> sbi->pipe->f_cred->user_ns;
>>>>>> +size_t name_offset;
>>>>>>  
>>>>>> -pktsz = sizeof(*packet);
>>>>>> +if (sbi->is32bit)
>>>>>> +name_offset = offsetof(struct autofs_v5_packet, 
>>>>>> len) +
>>>>>> +  sizeof(packet->len);
>>>>>> +else
>>>>>> +name_offset = offsetof(struct autofs_v5_packet, 
>>>>>> name);
>>>>>
>>>>> This doesn't help at all because the offset of struct 
>>>>> autofs_v5_packet.name
>>>>> does not change.
>>>>>
>>>>>> +pktsz = name_offset + sizeof(packet->name);
>>>>>
>>>>> What changes is pktsz: it's either sizeof(struct autofs_v5_packet)
>>>>> or 4 bytes less, depending on the architecture.
>>>>
>>>> Indeed. Thanks!
>>>>
>>>>> For example,
>>>>>
>>>>> #ifdef CONFIG_COMPAT
>>>>>   if (__alignof__(compat_u64) < __alignof__(u64) && sbi->is32bit)
>>>>
>>>> Unfortunately I don't get this "alignof" checks.
>>>> The intention of "is32bit" was to define this compat case completely.
>>>> Why are they required?
>>>
>>> On some 32-bit architectures like arm, u64 is 64-bit aligned, on others
>>> like x86 it is not.  This alignof check ensures that compat 32-bit
>>> architectures with 64-bit alignment are not going to be broken
>>> by the change.
>>
>> Thanks for the explanation!
>> But looks like the issue is hidden so deep (thanks to O_DIRECT), that 
>> becomes unimportant.
> 
> I'm not quite sure how O_DIRECT makes this unimportant.
> For example, automount_dispatch_io from systemd tries to read exactly
> sizeof(union autofs_v5_packet_union) bytes and fails in case of short read.
> 

That's weird. Can you reproduce it?
The problem is that kernel always send arch packet (i.e. 300 bytes for x86, or 
304 for x86_64).
So, how can systemd get less? It can get more, but only if it'll try to read 
more.
Otherwise, as "man pipe(2)" says:

*  If a read(2) specifies a buffer size that is smaller than the next packet, 
then the requested number of bytes are read, and the excess bytes in the packet 
are discarded.

> 
> 
> 
> ___
> Devel mailing list
> Devel@openvz.org
> https://lists.openvz.org/mailman/listinfo/devel
> 
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [RFC PATCH 2/2] autofs: sent 32-bit sized packet for 32-bit process

[Stanislav Kinsburskiy]

01.09.2017 16:53, Dmitry V. Levin P?P8QP5Q:
> On Fri, Sep 01, 2017 at 12:15:17PM +0300, Stanislav Kinsburskiy wrote:
>> 31.08.2017 20:22, Dmitry V. Levin P?P8QP5Q:
>>> On Thu, Aug 31, 2017 at 05:57:11PM +0400, Stanislav Kinsburskiy wrote:
>>>> The structure autofs_v5_packet (except name) is not aligned by 8 bytes, 
>>>> which
>>>> lead to different sizes in 32 and 64-bit architectures.
>>>> Let's form 32-bit compatible packet when daemon has 32-bit addressation.
>>>>
>>>> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
>>>> ---
>>>>  fs/autofs4/waitq.c |   11 +--
>>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
>>>> index 309ca6b..484cf2e 100644
>>>> --- a/fs/autofs4/waitq.c
>>>> +++ b/fs/autofs4/waitq.c
>>>> @@ -153,12 +153,19 @@ static void autofs4_notify_daemon(struct 
>>>> autofs_sb_info *sbi,
>>>>{
>>>>struct autofs_v5_packet *packet = _pkt.v5_packet;
>>>>struct user_namespace *user_ns = sbi->pipe->f_cred->user_ns;
>>>> +  size_t name_offset;
>>>>  
>>>> -  pktsz = sizeof(*packet);
>>>> +  if (sbi->is32bit)
>>>> +  name_offset = offsetof(struct autofs_v5_packet, len) +
>>>> +sizeof(packet->len);
>>>> +  else
>>>> +  name_offset = offsetof(struct autofs_v5_packet, name);
>>>
>>> This doesn't help at all because the offset of struct autofs_v5_packet.name
>>> does not change.
>>>
>>>> +  pktsz = name_offset + sizeof(packet->name);
>>>
>>> What changes is pktsz: it's either sizeof(struct autofs_v5_packet)
>>> or 4 bytes less, depending on the architecture.
>>
>> Indeed. Thanks!
>>
>>> For example,
>>>
>>> #ifdef CONFIG_COMPAT
>>> if (__alignof__(compat_u64) < __alignof__(u64) && sbi->is32bit)
>>
>> Unfortunately I don't get this "alignof" checks.
>> The intention of "is32bit" was to define this compat case completely.
>> Why are they required?
> 
> On some 32-bit architectures like arm, u64 is 64-bit aligned, on others
> like x86 it is not.  This alignof check ensures that compat 32-bit
> architectures with 64-bit alignment are not going to be broken
> by the change.
> 

Thanks for the explanation!
But looks like the issue is hidden so deep (thanks to O_DIRECT), that becomes 
unimportant.

> 
> 
> 
> ___
> Devel mailing list
> Devel@openvz.org
> https://lists.openvz.org/mailman/listinfo/devel
> 
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] zdtm: fix autofs to work with autofs_v5_packet_union issue

[Stanislav Kinsburskiy]

The only patch in the series discards the following CRIU commits:

27ce0613e7319c97cc7bfc0a240e3f5f61f3d912
e90b5ed57af866010bc8f5c7c9730aa68995cb77
2fc59ffe3ead2eff44542a415f16b1559e2c8140

The following series implements...

---

Stanislav Kinsburskiy (1):
  tests: do not try to read more than packet in AutoFS test


 test/zdtm/static/autofs.c |   13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] tests: do not try to read more than packet in AutoFS test

[Stanislav Kinsburskiy]

The intention was to make sure, that only one packet is sent at a time.
And thus read has to return exactly the size of one packet.
But it doesnt' work as expected, because size of autofs_v5_packet_union
differs on 32 bit and 64 bit architectures.
This is a bug, but it's hidden so deeply, that no one really cares by the
following 2 aspects:
1) Autofs pipe has O_DIRECT flag, which means excess bytes will be discarded
upon read.
2) No one tries to read more than one packet at a time.

So let's fix the test instead and do not try to read more bytes, than
expected.

https://jira.sw.ru/browse/PSBM-71078

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 test/zdtm/static/autofs.c |   13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/test/zdtm/static/autofs.c b/test/zdtm/static/autofs.c
index 747ab69..ae78538 100644
--- a/test/zdtm/static/autofs.c
+++ b/test/zdtm/static/autofs.c
@@ -460,7 +460,7 @@ static int automountd_loop(int pipe, const char 
*mountpoint, struct autofs_param
 {
union autofs_v5_packet_union *packet;
ssize_t bytes;
-   size_t psize = sizeof(*packet) + 1;
+   size_t psize = sizeof(*packet);
int err = 0;
 
packet = malloc(psize);
@@ -473,7 +473,7 @@ static int automountd_loop(int pipe, const char 
*mountpoint, struct autofs_param
siginterrupt(SIGUSR2, 1);
 
while (!stop && !err) {
-   memset(packet, 0, sizeof(*packet));
+   memset(packet, 0, psize);
 
bytes = read(pipe, packet, psize);
if (bytes < 0) {
@@ -483,12 +483,9 @@ static int automountd_loop(int pipe, const char 
*mountpoint, struct autofs_param
}
continue;
}
-   if (bytes == psize) {
-   pr_err("read more that expected\n");
-   return -EINVAL;
-   }
-   if (bytes != sizeof(*packet)) {
-   pr_err("read less than expected: %zd\n", bytes);
+   if (bytes != psize) {
+   pr_err("read less than expected: %zd < %zd\n",
+   bytes, psize);
return -EINVAL;
}
err = automountd_serve(mountpoint, param, packet);

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [RFC PATCH 2/2] autofs: sent 32-bit sized packet for 32-bit process

[Stanislav Kinsburskiy]

The structure autofs_v5_packet (except name) is not aligned by 8 bytes, which
leads to different sizes in 32 and 64-bit architectures.
Let's form 32-bit compatible packet when daemon has 32-bit addressation.

Suggested-by: Dmitry V. Levin <l...@altlinux.org>
Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 fs/autofs4/waitq.c |5 +
 1 file changed, 5 insertions(+)

diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index 24a58bf..1f9b7d8 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -151,6 +151,11 @@ static void autofs4_notify_daemon(struct autofs_sb_info 
*sbi,
struct autofs_v5_packet *packet = _pkt.v5_packet;
struct user_namespace *user_ns = sbi->pipe->f_cred->user_ns;
 
+#ifdef CONFIG_COMPAT
+   if (sbi->is32bit)
+   pktsz = offsetofend(struct autofs_v5_packet, name);
+   else
+#endif
pktsz = sizeof(*packet);
 
packet->wait_queue_token = wq->wait_queue_token;

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation

[Stanislav Kinsburskiy]

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 fs/autofs4/autofs_i.h  |3 +++
 fs/autofs4/dev-ioctl.c |3 +++
 fs/autofs4/inode.c |4 +++-
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index 4737615..3da105f 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -120,6 +120,9 @@ struct autofs_sb_info {
struct list_head active_list;
struct list_head expiring_list;
struct rcu_head rcu;
+#ifdef CONFIG_COMPAT
+   unsigned is32bit:1;
+#endif
 };
 
 static inline struct autofs_sb_info *autofs4_sbi(struct super_block *sb)
diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
index b7c816f..467d6c4 100644
--- a/fs/autofs4/dev-ioctl.c
+++ b/fs/autofs4/dev-ioctl.c
@@ -397,6 +397,9 @@ static int autofs_dev_ioctl_setpipefd(struct file *fp,
sbi->pipefd = pipefd;
sbi->pipe = pipe;
sbi->catatonic = 0;
+#ifdef CONFIG_COMPAT
+   sbi->is32bit = is_compat_task();
+#endif
}
 out:
put_pid(new_pid);
diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
index 09e7d68..21d3c0b 100644
--- a/fs/autofs4/inode.c
+++ b/fs/autofs4/inode.c
@@ -301,7 +301,9 @@ int autofs4_fill_super(struct super_block *s, void *data, 
int silent)
} else {
sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
}
-
+#ifdef CONFIG_COMPAT
+   sbi->is32bit = is_compat_task();
+#endif
if (autofs_type_trigger(sbi->type))
__managed_dentry_set_managed(root);
 

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [RFC PATCH 0/2] autofs: fix autofs_v5_packet dlivery in compat mode

[Stanislav Kinsburskiy]

The problem is that in compat mode struct autofs_v5_packet has to have 
different size
(i.e. 4 bytes less).

This is RFC because:
1) This issue is hidden, because autofs pipe has O_DIRECT and the rest of the
epacket is truncated when read.
2) X86 arch doesn't have is_compat_task() helper
3) It's now clear, what to do if "pgrp" option is specified.

The following series implements...

---

Stanislav Kinsburskiy (2):
  autofs: set compat flag on sbi when daemon uses 32bit addressation
  autofs: sent 32-bit sized packet for 32-bit process


 fs/autofs4/autofs_i.h  |3 +++
 fs/autofs4/dev-ioctl.c |3 +++
 fs/autofs4/inode.c |4 +++-
 fs/autofs4/waitq.c |5 +
 4 files changed, 14 insertions(+), 1 deletion(-)
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] autofs: fix leaked pid on error path in autofs4_fill_super

[Stanislav Kinsburskiy]

Check for protocol happens after pid get.

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 fs/autofs4/inode.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
index 0ba9c02..2cd4e7e 100644
--- a/fs/autofs4/inode.c
+++ b/fs/autofs4/inode.c
@@ -298,7 +298,7 @@ int autofs4_fill_super(struct super_block *s, void *data, 
int silent)
   "daemon (%d, %d) kernel (%d, %d)\n",
sbi->min_proto, sbi->max_proto,
AUTOFS_MIN_PROTO_VERSION, AUTOFS_MAX_PROTO_VERSION);
-   goto fail_dput;
+   goto fail_put_pid;
}
 
/* Establish highest kernel protocol version */

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] autofs: fix double pid put in error path

[Stanislav Kinsburskiy]

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 fs/autofs4/inode.c |1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
index b23cf2a..0ba9c02 100644
--- a/fs/autofs4/inode.c
+++ b/fs/autofs4/inode.c
@@ -343,7 +343,6 @@ int autofs4_fill_super(struct super_block *s, void *data, 
int silent)
 fail_ino:
kfree(ino);
 fail_free:
-   put_pid(sbi->oz_pgrp);
kfree(sbi);
s->s_fs_info = NULL;
return ret;

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [RFC PATCH 2/2] autofs: sent 32-bit sized packet for 32-bit process

[Stanislav Kinsburskiy]

31.08.2017 20:22, Dmitry V. Levin пишет:
> On Thu, Aug 31, 2017 at 05:57:11PM +0400, Stanislav Kinsburskiy wrote:
>> The structure autofs_v5_packet (except name) is not aligned by 8 bytes, which
>> lead to different sizes in 32 and 64-bit architectures.
>> Let's form 32-bit compatible packet when daemon has 32-bit addressation.
>>
>> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
>> ---
>>  fs/autofs4/waitq.c |   11 +--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
>> index 309ca6b..484cf2e 100644
>> --- a/fs/autofs4/waitq.c
>> +++ b/fs/autofs4/waitq.c
>> @@ -153,12 +153,19 @@ static void autofs4_notify_daemon(struct 
>> autofs_sb_info *sbi,
>>  {
>>  struct autofs_v5_packet *packet = _pkt.v5_packet;
>>  struct user_namespace *user_ns = sbi->pipe->f_cred->user_ns;
>> +size_t name_offset;
>>  
>> -pktsz = sizeof(*packet);
>> +if (sbi->is32bit)
>> +name_offset = offsetof(struct autofs_v5_packet, len) +
>> +  sizeof(packet->len);
>> +else
>> +name_offset = offsetof(struct autofs_v5_packet, name);
> 
> This doesn't help at all because the offset of struct autofs_v5_packet.name
> does not change.
> 
>> +pktsz = name_offset + sizeof(packet->name);
> 
> What changes is pktsz: it's either sizeof(struct autofs_v5_packet)
> or 4 bytes less, depending on the architecture.

Indeed. Thanks!

> For example,
> 
> #ifdef CONFIG_COMPAT
>   if (__alignof__(compat_u64) < __alignof__(u64) && sbi->is32bit)

Unfortunately I don't get this "alignof" checks.
The intention of "is32bit" was to define this compat case completely.
Why are they required?

>   pktsz = offsetofend(struct autofs_v5_packet, name);
>   else
> #endif
>   pktsz = sizeof(*packet);
> 
> 



___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [RFC PATCH 2/2] autofs: sent 32-bit sized packet for 32-bit process

[Stanislav Kinsburskiy]

The structure autofs_v5_packet (except name) is not aligned by 8 bytes, which
lead to different sizes in 32 and 64-bit architectures.
Let's form 32-bit compatible packet when daemon has 32-bit addressation.

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 fs/autofs4/waitq.c |   11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index 309ca6b..484cf2e 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -153,12 +153,19 @@ static void autofs4_notify_daemon(struct autofs_sb_info 
*sbi,
{
struct autofs_v5_packet *packet = _pkt.v5_packet;
struct user_namespace *user_ns = sbi->pipe->f_cred->user_ns;
+   size_t name_offset;
 
-   pktsz = sizeof(*packet);
+   if (sbi->is32bit)
+   name_offset = offsetof(struct autofs_v5_packet, len) +
+ sizeof(packet->len);
+   else
+   name_offset = offsetof(struct autofs_v5_packet, name);
+
+   pktsz = name_offset + sizeof(packet->name);
 
packet->wait_queue_token = wq->wait_queue_token;
packet->len = wq->name.len;
-   memcpy(packet->name, wq->name.name, wq->name.len);
+   memcpy(packet + name_offset, wq->name.name, wq->name.len);
packet->name[wq->name.len] = '\0';
packet->dev = wq->dev;
packet->ino = wq->ino;

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation

[Stanislav Kinsburskiy]

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 fs/autofs4/inode.c |   16 
 1 file changed, 16 insertions(+)

diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
index b23cf2a..989ac38 100644
--- a/fs/autofs4/inode.c
+++ b/fs/autofs4/inode.c
@@ -217,6 +217,7 @@ int autofs4_fill_super(struct super_block *s, void *data, 
int silent)
int pgrp;
bool pgrp_set = false;
int ret = -EINVAL;
+   struct task_struct *tsk;
 
sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
if (!sbi)
@@ -281,10 +282,25 @@ int autofs4_fill_super(struct super_block *s, void *data, 
int silent)
pgrp);
goto fail_dput;
}
+   tsk = get_pid_task(sbi->oz_pgrp, PIDTYPE_PGID);
+   if (!tsk) {
+   pr_warn("autofs: could not find process group leader 
%d\n",
+   pgrp);
+   goto fail_put_pid;
+   }
} else {
sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
+   get_task_struct(current);
+   tsk = current;
}
 
+   if (test_tsk_thread_flag(tsk, TIF_ADDR32))
+   sbi->is32bit = 1;
+   else
+   sbi->is32bit = 0;
+
+   put_task_struct(tsk);
+
if (autofs_type_trigger(sbi->type))
__managed_dentry_set_managed(root);
 

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [RFC PATCH 0/2] autofs: add "compat" support

[Stanislav Kinsburskiy]

The idea is simple: reduce autofs_v5_packet for 32bit damon on 64bit
architectures.

---

Stanislav Kinsburskiy (2):
  autofs: set compat flag on sbi when daemon uses 32bit addressation
  autofs: sent 32-bit sized packet for 32-bit process


 fs/autofs4/inode.c |   16 
 fs/autofs4/waitq.c |   11 +--
 2 files changed, 25 insertions(+), 2 deletions(-)

--
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] autofs: fix autofs_v5_packet structure for compat mode

[Stanislav Kinsburskiy]

31.08.2017 15:05, Dmitry V. Levin пишет:
> On Thu, Aug 31, 2017 at 02:40:23PM +0300, Dmitry V. Levin wrote:
>> On Thu, Aug 31, 2017 at 01:48:27PM +0300, Stanislav Kinsburskiy wrote:
>>>
>>>
>>> 31.08.2017 13:38, Dmitry V. Levin пишет:
>>>> On Thu, Aug 31, 2017 at 02:11:34PM +0400, Stanislav Kinsburskiy wrote:
>>>>> Due to integer variables alignment size of struct autofs_v5_packet in 300
>>>>> bytes in 32-bit architectures (instead of 304 bytes in 64-bits 
>>>>> architectures).
>>>>>
>>>>> This may lead to memory corruption (64 bits kernel always send 304 bytes,
>>>>> while 32-bit userspace application expects for 300).
>>>>>
>>>>> https://jira.sw.ru/browse/PSBM-71078
>>>>>
>>>>> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
>>>>> ---
>>>>>  include/uapi/linux/auto_fs4.h |2 ++
>>>>>  1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/include/uapi/linux/auto_fs4.h b/include/uapi/linux/auto_fs4.h
>>>>> index e02982f..8729a47 100644
>>>>> --- a/include/uapi/linux/auto_fs4.h
>>>>> +++ b/include/uapi/linux/auto_fs4.h
>>>>> @@ -137,6 +137,8 @@ struct autofs_v5_packet {
>>>>>   __u32 pid;
>>>>>   __u32 tgid;
>>>>>   __u32 len;
>>>>> + __u32 blob; /* This is needed to align structure up to 8
>>>>> +bytes for ALL archs including 32-bit */
>>>>>   char name[NAME_MAX+1];
>>>>>  };
>>>>
>>>> This change breaks ABI because it changes offsetof(struct 
>>>> autofs_v5_packet, name).
>>>> If you need to fix the alignment, use  __attribute__((aligned(8))).
>>>>
>>>
>>> Nice to know you're watching.
>>> Yes, attribute is better.
>>> But how ABI is broken? On x86_64 this alignment is implied, so nothing is 
>>> changed.
>>
>> Your change increases offsetof(struct autofs_v5_packet, name) by 4 on all
>> architectures.  On architectures where the structure is 32-bit aligned
>> this also leads to increase of its size by 4.
>>
>>>> An alignment change would also be an ABI breakage on 32-bit architectures,
>>>> though.
>>>>
>>>
>>> True.
>>> But from my POW better have it working on 64bit archs for 32bit apps.
>>> But anyway, upstream guys will device, whether they want 32-bit autofs 
>>> applications properly work on 64 or 32 bits.
>>
>> Let's fix old bugs without introducing new bugs.
>> The right fix here seems to be a compat structure, that is, both 64-bit
>> and 32-bit kernels should send the same 32-bit aligned structure, and
>> it has to be the same structure sent by traditional 32-bit kernels.
> 
> Alternatively, a much more simple fix would be to change 64-bit kernels
> not to send the trailing 4 padding bytes of 64-bit aligned
> struct autofs_v5_packet.  That is, just send
> offsetofend(struct autofs_v5_packet, name) bytes instead of
> sizeof(struct autofs_v5_packet) regardless of architecture.
> 

Fair enough, thanks!
But this approach won't work, because autofs pipe has O_DIRECT flag.
Compat structure looks more promising, but not yet clear to me, how to define 
one properly.
Probably by replacing "len" with char array like this:

/* autofs v5 common packet struct */
struct autofs_v5_packet {
struct autofs_packet_hdr hdr;
autofs_wqt_t wait_queue_token;
__u32 dev;
__u64 ino;
__u32 uid;
__u32 gid;
__u32 pid;
__u32 tgid;
__u8  len[4];
char name[NAME_MAX+1];
};

What do you think?
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] autofs: fix autofs_v5_packet structure for compat mode

[Stanislav Kinsburskiy]

31.08.2017 13:38, Dmitry V. Levin пишет:
> On Thu, Aug 31, 2017 at 02:11:34PM +0400, Stanislav Kinsburskiy wrote:
>> Due to integer variables alignment size of struct autofs_v5_packet in 300
>> bytes in 32-bit architectures (instead of 304 bytes in 64-bits 
>> architectures).
>>
>> This may lead to memory corruption (64 bits kernel always send 304 bytes,
>> while 32-bit userspace application expects for 300).
>>
>> https://jira.sw.ru/browse/PSBM-71078
>>
>> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
>> ---
>>  include/uapi/linux/auto_fs4.h |2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/include/uapi/linux/auto_fs4.h b/include/uapi/linux/auto_fs4.h
>> index e02982f..8729a47 100644
>> --- a/include/uapi/linux/auto_fs4.h
>> +++ b/include/uapi/linux/auto_fs4.h
>> @@ -137,6 +137,8 @@ struct autofs_v5_packet {
>>  __u32 pid;
>>  __u32 tgid;
>>  __u32 len;
>> +__u32 blob; /* This is needed to align structure up to 8
>> +   bytes for ALL archs including 32-bit */
>>  char name[NAME_MAX+1];
>>  };
> 
> This change breaks ABI because it changes offsetof(struct autofs_v5_packet, 
> name).
> If you need to fix the alignment, use  __attribute__((aligned(8))).
> 

Nice to know you're watching.
Yes, attribute is better.
But how ABI is broken? On x86_64 this alignment is implied, so nothing is 
changed.

> An alignment change would also be an ABI breakage on 32-bit architectures,
> though.
> 

True.
But from my POW better have it working on 64bit archs for 32bit apps.
But anyway, upstream guys will device, whether they want 32-bit autofs 
applications properly work on 64 or 32 bits.
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] autofs: fix autofs_v5_packet structure for compat mode

[Stanislav Kinsburskiy]

Yes

31.08.2017 13:37, Konstantin Khorenko пишет:
> Will you send it to mainstream as well?
> 
> -- 
> Best regards,
> 
> Konstantin Khorenko,
> Virtuozzo Linux Kernel Team
> 
> On 08/31/2017 01:11 PM, Stanislav Kinsburskiy wrote:
>> Due to integer variables alignment size of struct autofs_v5_packet in 300
>> bytes in 32-bit architectures (instead of 304 bytes in 64-bits 
>> architectures).
>>
>> This may lead to memory corruption (64 bits kernel always send 304 bytes,
>> while 32-bit userspace application expects for 300).
>>
>> https://jira.sw.ru/browse/PSBM-71078
>>
>> Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
>> ---
>>  include/uapi/linux/auto_fs4.h |2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/include/uapi/linux/auto_fs4.h b/include/uapi/linux/auto_fs4.h
>> index e02982f..8729a47 100644
>> --- a/include/uapi/linux/auto_fs4.h
>> +++ b/include/uapi/linux/auto_fs4.h
>> @@ -137,6 +137,8 @@ struct autofs_v5_packet {
>>  __u32 pid;
>>  __u32 tgid;
>>  __u32 len;
>> +__u32 blob;/* This is needed to align structure up to 8
>> +   bytes for ALL archs including 32-bit */
>>  char name[NAME_MAX+1];
>>  };
>>
>>
>> .
>>
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] autofs: fix autofs_v5_packet structure for compat mode

[Stanislav Kinsburskiy]

Due to integer variables alignment size of struct autofs_v5_packet in 300
bytes in 32-bit architectures (instead of 304 bytes in 64-bits architectures).

This may lead to memory corruption (64 bits kernel always send 304 bytes,
while 32-bit userspace application expects for 300).

https://jira.sw.ru/browse/PSBM-71078

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 include/uapi/linux/auto_fs4.h |2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/uapi/linux/auto_fs4.h b/include/uapi/linux/auto_fs4.h
index e02982f..8729a47 100644
--- a/include/uapi/linux/auto_fs4.h
+++ b/include/uapi/linux/auto_fs4.h
@@ -137,6 +137,8 @@ struct autofs_v5_packet {
__u32 pid;
__u32 tgid;
__u32 len;
+   __u32 blob; /* This is needed to align structure up to 8
+  bytes for ALL archs including 32-bit */
char name[NAME_MAX+1];
 };
 

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] zdtm: fix package memory allocation in autofs.c

[Stanislav Kinsburskiy]

Plus some cleanup.

https://jira.sw.ru/browse/PSBM-71078

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 test/zdtm/static/autofs.c |   18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/test/zdtm/static/autofs.c b/test/zdtm/static/autofs.c
index 8d917ee..882289f 100644
--- a/test/zdtm/static/autofs.c
+++ b/test/zdtm/static/autofs.c
@@ -460,10 +460,10 @@ static int automountd_loop(int pipe, const char 
*mountpoint, struct autofs_param
 {
union autofs_v5_packet_union *packet;
ssize_t bytes;
-   size_t psize = sizeof(*packet) * 2;
+   size_t psize = sizeof(*packet);
int err = 0;
 
-   packet = malloc(psize);
+   packet = malloc(psize * 2);
if (!packet) {
pr_err("failed to allocate autofs packet\n");
return -ENOMEM;
@@ -473,7 +473,7 @@ static int automountd_loop(int pipe, const char 
*mountpoint, struct autofs_param
siginterrupt(SIGUSR2, 1);
 
while (!stop && !err) {
-   memset(packet, 0, sizeof(*packet));
+   memset(packet, 0, psize * 2);
 
bytes = read(pipe, packet, psize);
if (bytes < 0) {
@@ -483,12 +483,12 @@ static int automountd_loop(int pipe, const char 
*mountpoint, struct autofs_param
}
continue;
}
-   if (bytes > psize) {
-   pr_err("read more that expected: %zd > %zd\n", bytes, 
psize);
-   return -EINVAL;
-   }
-   if (bytes != sizeof(*packet)) {
-   pr_err("read less than expected: %zd\n", bytes);
+   if (bytes != psize) {
+   pr_err("read %s that expected: %zd %s %zd\n",
+   (bytes > psize) ? "more" : "less",
+   bytes,
+   (bytes > psize) ? ">" : "<",
+   psize);
return -EINVAL;
}
err = automountd_serve(mountpoint, param, packet);

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] zdtm: fix autofs tes compilation

[Stanislav Kinsburskiy]

Variables type of size_t have to be printed with "%z" directive.

https://jira.sw.ru/browse/PSBM-71041

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 test/zdtm/static/autofs.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/zdtm/static/autofs.c b/test/zdtm/static/autofs.c
index d854fd0..8d917ee 100644
--- a/test/zdtm/static/autofs.c
+++ b/test/zdtm/static/autofs.c
@@ -484,7 +484,7 @@ static int automountd_loop(int pipe, const char 
*mountpoint, struct autofs_param
continue;
}
if (bytes > psize) {
-   pr_err("read more that expected: %ld > %ld\n", bytes, 
psize);
+   pr_err("read more that expected: %zd > %zd\n", bytes, 
psize);
return -EINVAL;
}
if (bytes != sizeof(*packet)) {

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH rh7 2/2] mm/memcg: reclaim only kmem if kmem limit reached.

[Stanislav Kinsburskiy]

25.08.2017 18:38, Andrey Ryabinin пишет:
> If kmem limit on memcg reached, we go into memory reclaim,
> and reclaim everything we can, including page cache and anon.
> Reclaiming page cache or anon won't help since we need to lower
> only kmem usage. This patch fixes the problem by avoiding
> non-kmem reclaim on hitting the kmem limit.
> 

Can't there be a situation, when some object in anon mem or page cache holds 
some object in kmem (indirectly)?

> https://jira.sw.ru/browse/PSBM-69226
> Signed-off-by: Andrey Ryabinin 
> ---
>  include/linux/memcontrol.h | 10 ++
>  include/linux/swap.h   |  2 +-
>  mm/memcontrol.c| 30 --
>  mm/vmscan.c| 31 ---
>  4 files changed, 51 insertions(+), 22 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 1a52e58ab7de..1d6bc80c4c90 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -45,6 +45,16 @@ struct mem_cgroup_reclaim_cookie {
>   unsigned int generation;
>  };
>  
> +/*
> + * Reclaim flags for mem_cgroup_hierarchical_reclaim
> + */
> +#define MEM_CGROUP_RECLAIM_NOSWAP_BIT0x0
> +#define MEM_CGROUP_RECLAIM_NOSWAP(1 << MEM_CGROUP_RECLAIM_NOSWAP_BIT)
> +#define MEM_CGROUP_RECLAIM_SHRINK_BIT0x1
> +#define MEM_CGROUP_RECLAIM_SHRINK(1 << MEM_CGROUP_RECLAIM_SHRINK_BIT)
> +#define MEM_CGROUP_RECLAIM_KMEM_BIT  0x2
> +#define MEM_CGROUP_RECLAIM_KMEM  (1 << 
> MEM_CGROUP_RECLAIM_KMEM_BIT)
> +
>  #ifdef CONFIG_MEMCG
>  int mem_cgroup_try_charge(struct page *page, struct mm_struct *mm,
> gfp_t gfp_mask, struct mem_cgroup **memcgp);
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index bd162f9bef0d..bd47451ec95a 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -324,7 +324,7 @@ extern unsigned long try_to_free_pages(struct zonelist 
> *zonelist, int order,
>  extern int __isolate_lru_page(struct page *page, isolate_mode_t mode);
>  extern unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *mem,
> unsigned long nr_pages,
> -   gfp_t gfp_mask, bool noswap);
> +   gfp_t gfp_mask, int flags);
>  extern unsigned long mem_cgroup_shrink_node_zone(struct mem_cgroup *mem,
>   gfp_t gfp_mask, bool noswap,
>   struct zone *zone,
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 97824e281d7a..f9a5f3819a31 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -511,16 +511,6 @@ enum res_type {
>  #define OOM_CONTROL  (0)
>  
>  /*
> - * Reclaim flags for mem_cgroup_hierarchical_reclaim
> - */
> -#define MEM_CGROUP_RECLAIM_NOSWAP_BIT0x0
> -#define MEM_CGROUP_RECLAIM_NOSWAP(1 << MEM_CGROUP_RECLAIM_NOSWAP_BIT)
> -#define MEM_CGROUP_RECLAIM_SHRINK_BIT0x1
> -#define MEM_CGROUP_RECLAIM_SHRINK(1 << MEM_CGROUP_RECLAIM_SHRINK_BIT)
> -#define MEM_CGROUP_RECLAIM_KMEM_BIT  0x2
> -#define MEM_CGROUP_RECLAIM_KMEM  (1 << 
> MEM_CGROUP_RECLAIM_KMEM_BIT)
> -
> -/*
>   * The memcg_create_mutex will be held whenever a new cgroup is created.
>   * As a consequence, any change that needs to protect against new child 
> cgroups
>   * appearing has to hold it as well.
> @@ -2137,7 +2127,7 @@ static unsigned long mem_cgroup_reclaim(struct 
> mem_cgroup *memcg,
>   if (loop)
>   drain_all_stock_async(memcg);
>   total += try_to_free_mem_cgroup_pages(memcg, SWAP_CLUSTER_MAX,
> -   gfp_mask, noswap);
> +   gfp_mask, flags);
>   if (test_thread_flag(TIF_MEMDIE) ||
>   fatal_signal_pending(current))
>   return 1;
> @@ -2150,6 +2140,16 @@ static unsigned long mem_cgroup_reclaim(struct 
> mem_cgroup *memcg,
>   break;
>   if (mem_cgroup_margin(memcg, flags & MEM_CGROUP_RECLAIM_KMEM))
>   break;
> +
> + /*
> +  * Try harder to reclaim dcache. dcache reclaim may
> +  * temporarly fail due to dcache->dlock being held
> +  * by someone else. We must try harder to avoid premature
> +  * slab allocation failures.
> +  */
> + if (flags & MEM_CGROUP_RECLAIM_KMEM &&
> + page_counter_read(>dcache))
> + continue;
>   /*
>* If nothing was reclaimed after two attempts, there
>* may be no reclaimable pages in this hierarchy.
> @@ -2778,11 +2778,13 @@ static int try_charge(struct mem_cgroup *memcg, gfp_t 
> gfp_mask, bool kmem_charge
>   struct mem_cgroup 

[Devel] [PATCH] zdtm: print autofs request size, if read more than expected

[Stanislav Kinsburskiy]

This is more debug patch, than fix. But valuable for debugging.

https://jira.sw.ru/browse/PSBM-70345

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 test/zdtm/static/autofs.c |6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/test/zdtm/static/autofs.c b/test/zdtm/static/autofs.c
index 747ab69..d854fd0 100644
--- a/test/zdtm/static/autofs.c
+++ b/test/zdtm/static/autofs.c
@@ -460,7 +460,7 @@ static int automountd_loop(int pipe, const char 
*mountpoint, struct autofs_param
 {
union autofs_v5_packet_union *packet;
ssize_t bytes;
-   size_t psize = sizeof(*packet) + 1;
+   size_t psize = sizeof(*packet) * 2;
int err = 0;
 
packet = malloc(psize);
@@ -483,8 +483,8 @@ static int automountd_loop(int pipe, const char 
*mountpoint, struct autofs_param
}
continue;
}
-   if (bytes == psize) {
-   pr_err("read more that expected\n");
+   if (bytes > psize) {
+   pr_err("read more that expected: %ld > %ld\n", bytes, 
psize);
return -EINVAL;
}
if (bytes != sizeof(*packet)) {

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [libvzctl PATCH] fast stop: suppress SUNRPC per task

[Stanislav Kinsburskiy]

21.08.2017 18:17, Igor Sukhih пишет:
> On 08/21/2017 04:18 PM, Stanislav Kinsburskiy wrote:
>> It solves two issues.
>> First, it allows to kill tasks in nested namespaces (rare, but possible 
>> case).
>> Second, is allows to kill CT with exited child reaper (when it's almost dead,
>> but waiting for it's children), because in this case SUNRPC can't be 
>> supressed
>> via child reapers proc dentries (/proc/pid/net is not available anymore).
>>
>> https://jira.sw.ru/browse/PSBM-70437
>>
>> Note: this patch reverts (replaces) commit
>> f8e953f5d683aa6f04dcc2b563b98d6e9ee3
>>
> Why it can't be done in scope of env cleanup logic in the kernel?

Because we have suspend and can't distinguish between halt and suspend.

> How it going to work if Ct stop initiated not by vzctl but by OOM, halt, kill?

It's the same.
The intention of this SUNRPC handle is to be able to stop container with any 
NFS issue.
For instance with unreachable network, or when power off was called in CT (in 
this case network is disabled before processes exit).


> 
> -- 
>   Igor.
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [libvzctl PATCH] cgrop: get process CT ID from its status

[Stanislav Kinsburskiy]

Taking CT ID from /proc//cgroup is not reliable.
For instance when "shutdown" in called inside CT, CTs init has the following
cgroup content:

root@skinsbursky-vz7:~/stub_proxy_fs# cat /proc/397761/cgroup
16:perf_event:/
15:cpuset:/
14:hugetlb:/
13:pids:/
10:ve:/
8:devices:/
7:freezer:/
6:net_prio,net_cls:/
5:cpuacct,cpu:/
4:name=systemd:/
3:beancounter:/
2:memory:/
1:blkio:/

This is because process exits cgroups before killing all it's descendants.
This leads to failing "cg_env_check_init_pid" (processes "CT ID" doesn't match
cached CT ID), leading in turn to skipping SUNRPC suppress, thus breaking CT
killing with unreachable NFS.

This patch changes the logic to take envID from /proc/pid/status, which
contains real VE ID, but not cgroup.

https://jira.sw.ru/browse/PSBM-70402

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 lib/cgroup.c |6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/cgroup.c b/lib/cgroup.c
index 843e197..cb521b6 100644
--- a/lib/cgroup.c
+++ b/lib/cgroup.c
@@ -798,11 +798,11 @@ int cg_env_set_net_classid(const char *ctid, unsigned int 
classid)
 
 static int cg_env_check_init_pid(const char *ctid, pid_t pid)
 {
-   int ret, n;
+   int ret;
FILE *fp;
char buf[4096];
 
-   snprintf(buf, sizeof(buf), "/proc/%d/cgroup", pid);
+   snprintf(buf, sizeof(buf), "/proc/%d/status", pid);
fp = fopen(buf, "r");
if (fp == NULL) {
if (errno == ENOENT)
@@ -813,7 +813,7 @@ static int cg_env_check_init_pid(const char *ctid, pid_t 
pid)
 
ret = 1;
while (fgets(buf, sizeof(buf), fp)) {
-   if (sscanf(buf, "%d:ve:/%s", , buf) != 2)
+   if (sscanf(buf, "envID:  %s", buf) != 1)
continue;
 
if (!strcmp(ctid, buf))

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH v2 00/27] proc connector: containerize on per-VE basis

[Stanislav Kinsburskiy]

Andrey, please.

17.08.2017 16:59, Stanislav Kinsburskiy пишет:
> This feature is requested by customer and needed by cgred service.
> 
> https://jira.sw.ru/browse/PSBM-60227
> 
> What's ne in v2:
> 1) Containerization is done on per-VE basis
> 2) Event in container is also sent to VE#0
> 
> 
> ---
> 
> Stanislav Kinsburskiy (27):
>   connector: remove redundant input callback from cn_dev
>   connector: store all private data on VE structure
>   connector: introduce VE-aware get_cdev() helper
>   connector: per-ve init and fini helpers introduced
>   connector: use device stored in VE
>   connector: per-ve helpers intoruduced
>   connector: take cn_already_initialized from VE
>   proc connector: generic proc_event_connector() helper introduced
>   proc connector: use generic event helper for fork event
>   proc connector: use generic event helper for exec event
>   proc connector: use generic event helper for id event
>   proc connector: use generic event helper for sid event
>   proc connector: use generic event helper for ptrace event
>   proc connector: use generic event helper for comm event
>   proc connector: use generic event helper for coredump event
>   proc connector: use generic event helper for exit event
>   proc connector: add pid namespace awareness
>   proc connector: add per-ve init and fini foutines
>   proc connector: call proc-related init and fini routines explicitly
>   proc connector: take number of listeners and per-cpu conters from VE
>   proc connector: pass VE to event fillers
>   proc connector: take namespaces from VE
>   proc connector: use per-ve netlink sender helper
>   proc connector: send events to both VEs if not in VE#0
>   connector: containerize "connector" proc entry
>   connector: take VE from socket upon callback
>   connector: add VE SS hook
> 
> 
>  drivers/connector/cn_proc.c   |  386 
> -
>  drivers/connector/connector.c |  155 
>  include/linux/connector.h |   20 ++
>  include/linux/ve.h|4 
>  4 files changed, 323 insertions(+), 242 deletions(-)
> 
> --
> ___
> Devel mailing list
> Devel@openvz.org
> https://lists.openvz.org/mailman/listinfo/devel
> 
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH v2 21/27] proc connector: pass VE to event fillers

[Stanislav Kinsburskiy]

Precursor patch. VE will be used later to get proper pid and user namespaces
for correct event generation.

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 drivers/connector/cn_proc.c |   36 +++-
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
index ff99f06..b66fde8 100644
--- a/drivers/connector/cn_proc.c
+++ b/drivers/connector/cn_proc.c
@@ -64,6 +64,7 @@ static struct cn_msg *cn_msg_fill(__u8 *buffer, struct 
ve_struct *ve,
  struct task_struct *task,
  int what, int cookie,
  bool (*fill_event)(struct proc_event *ev,
+struct ve_struct *ve,
 struct task_struct *task,
 int cookie))
 {
@@ -85,7 +86,7 @@ static struct cn_msg *cn_msg_fill(__u8 *buffer, struct 
ve_struct *ve,
ev->timestamp_ns = timespec_to_ns();
ev->what = what;
 
-   return fill_event(ev, task, cookie) ? msg : NULL;
+   return fill_event(ev, ve, task, cookie) ? msg : NULL;
 }
 
 static int proc_event_num_listeners(struct ve_struct *ve)
@@ -98,6 +99,7 @@ static int proc_event_num_listeners(struct ve_struct *ve)
 static void proc_event_connector(struct task_struct *task,
 int what, int cookie,
 bool (*fill_event)(struct proc_event *ev,
+   struct ve_struct *ve,
struct task_struct *task,
int cookie))
 {
@@ -116,8 +118,8 @@ static void proc_event_connector(struct task_struct *task,
cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
 }
 
-static bool fill_fork_event(struct proc_event *ev, struct task_struct *task,
-   int unused)
+static bool fill_fork_event(struct proc_event *ev, struct ve_struct *ve,
+   struct task_struct *task, int unused)
 {
struct task_struct *parent;
 
@@ -136,8 +138,8 @@ void proc_fork_connector(struct task_struct *task)
proc_event_connector(task, PROC_EVENT_FORK, 0, fill_fork_event);
 }
 
-static bool fill_exec_event(struct proc_event *ev, struct task_struct *task,
-   int unused)
+static bool fill_exec_event(struct proc_event *ev, struct ve_struct *ve,
+   struct task_struct *task, int unused)
 {
ev->event_data.exec.process_pid = task_pid_nr_ns(task, _pid_ns);
ev->event_data.exec.process_tgid = task_tgid_nr_ns(task, _pid_ns);
@@ -149,8 +151,8 @@ void proc_exec_connector(struct task_struct *task)
proc_event_connector(task, PROC_EVENT_EXEC, 0, fill_exec_event);
 }
 
-static bool fill_id_event(struct proc_event *ev, struct task_struct *task,
- int which_id)
+static bool fill_id_event(struct proc_event *ev, struct ve_struct *ve,
+ struct task_struct *task, int which_id)
 {
const struct cred *cred;
 
@@ -177,8 +179,8 @@ void proc_id_connector(struct task_struct *task, int 
which_id)
proc_event_connector(task, which_id, which_id, fill_id_event);
 }
 
-static bool fill_sid_event(struct proc_event *ev, struct task_struct *task,
-  int unused)
+static bool fill_sid_event(struct proc_event *ev, struct ve_struct *ve,
+  struct task_struct *task, int unused)
 {
ev->event_data.sid.process_pid = task_pid_nr_ns(task, _pid_ns);
ev->event_data.sid.process_tgid = task_tgid_nr_ns(task, _pid_ns);
@@ -190,8 +192,8 @@ void proc_sid_connector(struct task_struct *task)
proc_event_connector(task, PROC_EVENT_SID, 0, fill_sid_event);
 }
 
-static bool fill_ptrace_event(struct proc_event *ev, struct task_struct *task,
-  int ptrace_id)
+static bool fill_ptrace_event(struct proc_event *ev, struct ve_struct *ve,
+ struct task_struct *task, int ptrace_id)
 {
ev->event_data.ptrace.process_pid  = task_pid_nr_ns(task, _pid_ns);
ev->event_data.ptrace.process_tgid = task_tgid_nr_ns(task, 
_pid_ns);
@@ -212,8 +214,8 @@ void proc_ptrace_connector(struct task_struct *task, int 
ptrace_id)
 fill_ptrace_event);
 }
 
-static bool fill_comm_event(struct proc_event *ev, struct task_struct *task,
-   int unused)
+static bool fill_comm_event(struct proc_event *ev, struct ve_struct *ve,
+   struct task_struct *task, int unused)
 {
ev->event_data.comm.process_pid  = task_pid_nr_ns(task, _pid_ns);
ev->event_data.comm.process_tgid = task_tgid_nr_ns(task, _pid_ns);
@@ -226,8 +228,8 @@ void pr

[Devel] [PATCH v2 22/27] proc connector: take namespaces from VE

[Stanislav Kinsburskiy]

Intead of hardcoded "init" namespaces.

Signed-off-by: Stanislav Kinsburskiy <skinsbur...@virtuozzo.com>
---
 drivers/connector/cn_proc.c |   69 ++-
 1 file changed, 42 insertions(+), 27 deletions(-)

diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
index b66fde8..df6553d 100644
--- a/drivers/connector/cn_proc.c
+++ b/drivers/connector/cn_proc.c
@@ -122,14 +122,15 @@ static bool fill_fork_event(struct proc_event *ev, struct 
ve_struct *ve,
struct task_struct *task, int unused)
 {
struct task_struct *parent;
+   struct pid_namespace *pid_ns = ve->ve_ns->pid_ns;
 
rcu_read_lock();
parent = rcu_dereference(task->real_parent);
-   ev->event_data.fork.parent_pid = task_pid_nr_ns(parent, _pid_ns);
-   ev->event_data.fork.parent_tgid = task_tgid_nr_ns(parent, _pid_ns);
+   ev->event_data.fork.parent_pid = task_pid_nr_ns(parent, pid_ns);
+   ev->event_data.fork.parent_tgid = task_tgid_nr_ns(parent, pid_ns);
rcu_read_unlock();
-   ev->event_data.fork.child_pid = task_pid_nr_ns(task, _pid_ns);
-   ev->event_data.fork.child_tgid = task_tgid_nr_ns(task, _pid_ns);
+   ev->event_data.fork.child_pid = task_pid_nr_ns(task, pid_ns);
+   ev->event_data.fork.child_tgid = task_tgid_nr_ns(task, pid_ns);
return true;
 }
 
@@ -141,8 +142,10 @@ void proc_fork_connector(struct task_struct *task)
 static bool fill_exec_event(struct proc_event *ev, struct ve_struct *ve,
struct task_struct *task, int unused)
 {
-   ev->event_data.exec.process_pid = task_pid_nr_ns(task, _pid_ns);
-   ev->event_data.exec.process_tgid = task_tgid_nr_ns(task, _pid_ns);
+   struct pid_namespace *pid_ns = ve->ve_ns->pid_ns;
+
+   ev->event_data.exec.process_pid = task_pid_nr_ns(task, pid_ns);
+   ev->event_data.exec.process_tgid = task_tgid_nr_ns(task, pid_ns);
return true;
 }
 
@@ -155,17 +158,19 @@ static bool fill_id_event(struct proc_event *ev, struct 
ve_struct *ve,
  struct task_struct *task, int which_id)
 {
const struct cred *cred;
+   struct pid_namespace *pid_ns = ve->ve_ns->pid_ns;
+   struct user_namespace *user_ns = ve->init_cred->user_ns;
 
-   ev->event_data.id.process_pid = task_pid_nr_ns(task, _pid_ns);
-   ev->event_data.id.process_tgid = task_tgid_nr_ns(task, _pid_ns);
+   ev->event_data.id.process_pid = task_pid_nr_ns(task, pid_ns);
+   ev->event_data.id.process_tgid = task_tgid_nr_ns(task, pid_ns);
rcu_read_lock();
cred = __task_cred(task);
if (which_id == PROC_EVENT_UID) {
-   ev->event_data.id.r.ruid = from_kuid_munged(_user_ns, 
cred->uid);
-   ev->event_data.id.e.euid = from_kuid_munged(_user_ns, 
cred->euid);
+   ev->event_data.id.r.ruid = from_kuid_munged(user_ns, cred->uid);
+   ev->event_data.id.e.euid = from_kuid_munged(user_ns, 
cred->euid);
} else if (which_id == PROC_EVENT_GID) {
-   ev->event_data.id.r.rgid = from_kgid_munged(_user_ns, 
cred->gid);
-   ev->event_data.id.e.egid = from_kgid_munged(_user_ns, 
cred->egid);
+   ev->event_data.id.r.rgid = from_kgid_munged(user_ns, cred->gid);
+   ev->event_data.id.e.egid = from_kgid_munged(user_ns, 
cred->egid);
} else {
rcu_read_unlock();
return false;
@@ -182,8 +187,10 @@ void proc_id_connector(struct task_struct *task, int 
which_id)
 static bool fill_sid_event(struct proc_event *ev, struct ve_struct *ve,
   struct task_struct *task, int unused)
 {
-   ev->event_data.sid.process_pid = task_pid_nr_ns(task, _pid_ns);
-   ev->event_data.sid.process_tgid = task_tgid_nr_ns(task, _pid_ns);
+   struct pid_namespace *pid_ns = ve->ve_ns->pid_ns;
+
+   ev->event_data.sid.process_pid = task_pid_nr_ns(task, pid_ns);
+   ev->event_data.sid.process_tgid = task_tgid_nr_ns(task, pid_ns);
return true;
 }
 
@@ -195,11 +202,13 @@ void proc_sid_connector(struct task_struct *task)
 static bool fill_ptrace_event(struct proc_event *ev, struct ve_struct *ve,
  struct task_struct *task, int ptrace_id)
 {
-   ev->event_data.ptrace.process_pid  = task_pid_nr_ns(task, _pid_ns);
-   ev->event_data.ptrace.process_tgid = task_tgid_nr_ns(task, 
_pid_ns);
+   struct pid_namespace *pid_ns = ve->ve_ns->pid_ns;
+
+   ev->event_data.ptrace.process_pid  = task_pid_nr_ns(task, pid_ns);
+   ev->event_data.ptrace.process_tgid = task_tgid_nr_ns(task, pid_ns);
if (ptrace_id == PTRACE_ATTACH) {
-   ev->event_data.ptrace.tracer_pid  = task_pid_nr_ns(current, 
_pid_ns);
-