[v7,01/15] sysctl: add CTL_FLAGS_HAS to mark successful sysctl_op request

Submitted by Pavel Tikhomirov on April 28, 2016, 4:38 p.m.

Details

Message ID 1461861543-10463-2-git-send-email-ptikhomirov@virtuozzo.com
State Rejected
Series "net/ipv6: c/r dev/default/all conf ops"
Headers show

Commit Message

Pavel Tikhomirov April 28, 2016, 4:38 p.m.
v4: replace separate has pointer to CTL_FLAGS_HAS flag, second part in
patch "net/ipv4: add net_conf_op to reuse for ipv6"
v6: define CTL_FLAGS_HAS
v7: also allow EIO on do_sysctl_op for optional sysctls like
stable_secret and fix sysctl file to close in error path

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

Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 criu/include/sysctl.h |  1 +
 criu/sysctl.c         | 21 +++++++++++++++++----
 2 files changed, 18 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/include/sysctl.h b/criu/include/sysctl.h
index b949a40..3611ef1 100644
--- a/criu/include/sysctl.h
+++ b/criu/include/sysctl.h
@@ -35,5 +35,6 @@  enum {
  * Some entries might be missing mark them as optional.
  */
 #define CTL_FLAGS_OPTIONAL	1
+#define CTL_FLAGS_HAS		2
 
 #endif /* __CR_SYSCTL_H__ */
diff --git a/criu/sysctl.c b/criu/sysctl.c
index 21ae4ce..60e52db 100644
--- a/criu/sysctl.c
+++ b/criu/sysctl.c
@@ -303,8 +303,13 @@  static int __userns_sysctl_op(void *arg, int proc_fd, pid_t pid)
 		close(nsfd);
 
 		for (i = 0; i < userns_req->nr_req; i++) {
-			if (do_sysctl_op(fds[i], reqs[i], op) < 0)
-				exit(1);
+			if (do_sysctl_op(fds[i], reqs[i], op) < 0) {
+				if (errno != EIO || !(req->flags & CTL_FLAGS_OPTIONAL))
+					exit(1);
+			} else {
+				/* mark sysctl in question exists */
+				req->flags |= CTL_FLAGS_HAS;
+			}
 		}
 
 		exit(0);
@@ -372,8 +377,16 @@  static int __nonuserns_sysctl_op(struct sysctl_req *req, size_t nr_req, int op)
 		}
 
 		ret = do_sysctl_op(fd, req, op);
-		if (ret)
-			goto out;
+		if (ret) {
+			if (errno != EIO || !(req->flags & CTL_FLAGS_OPTIONAL)) {
+				close(fd);
+				goto out;
+			}
+		} else {
+			/* mark sysctl in question exists */
+			req->flags |= CTL_FLAGS_HAS;
+		}
+
 		close(fd);
 		req++;
 	}

Comments

Pavel Emelianov May 6, 2016, 11:47 a.m.
On 04/28/2016 07:38 PM, Pavel Tikhomirov wrote:
> v4: replace separate has pointer to CTL_FLAGS_HAS flag, second part in
> patch "net/ipv4: add net_conf_op to reuse for ipv6"
> v6: define CTL_FLAGS_HAS
> v7: also allow EIO on do_sysctl_op for optional sysctls like
> stable_secret and fix sysctl file to close in error path

Wait a second. If we've managed to open a sysctl file, but failed to write into
it, why should we pretend as if there was not this sysctl at all?

> 
> https://jira.sw.ru/browse/PSBM-30942
> 
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> ---
>  criu/include/sysctl.h |  1 +
>  criu/sysctl.c         | 21 +++++++++++++++++----
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/criu/include/sysctl.h b/criu/include/sysctl.h
> index b949a40..3611ef1 100644
> --- a/criu/include/sysctl.h
> +++ b/criu/include/sysctl.h
> @@ -35,5 +35,6 @@ enum {
>   * Some entries might be missing mark them as optional.
>   */
>  #define CTL_FLAGS_OPTIONAL	1
> +#define CTL_FLAGS_HAS		2
>  
>  #endif /* __CR_SYSCTL_H__ */
> diff --git a/criu/sysctl.c b/criu/sysctl.c
> index 21ae4ce..60e52db 100644
> --- a/criu/sysctl.c
> +++ b/criu/sysctl.c
> @@ -303,8 +303,13 @@ static int __userns_sysctl_op(void *arg, int proc_fd, pid_t pid)
>  		close(nsfd);
>  
>  		for (i = 0; i < userns_req->nr_req; i++) {
> -			if (do_sysctl_op(fds[i], reqs[i], op) < 0)
> -				exit(1);
> +			if (do_sysctl_op(fds[i], reqs[i], op) < 0) {
> +				if (errno != EIO || !(req->flags & CTL_FLAGS_OPTIONAL))
> +					exit(1);
> +			} else {
> +				/* mark sysctl in question exists */
> +				req->flags |= CTL_FLAGS_HAS;
> +			}
>  		}
>  
>  		exit(0);
> @@ -372,8 +377,16 @@ static int __nonuserns_sysctl_op(struct sysctl_req *req, size_t nr_req, int op)
>  		}
>  
>  		ret = do_sysctl_op(fd, req, op);
> -		if (ret)
> -			goto out;
> +		if (ret) {
> +			if (errno != EIO || !(req->flags & CTL_FLAGS_OPTIONAL)) {
> +				close(fd);
> +				goto out;
> +			}
> +		} else {
> +			/* mark sysctl in question exists */
> +			req->flags |= CTL_FLAGS_HAS;
> +		}
> +
>  		close(fd);
>  		req++;
>  	}
>
Pavel Tikhomirov May 6, 2016, 12:10 p.m.
On 05/06/2016 02:47 PM, Pavel Emelyanov wrote:
> On 04/28/2016 07:38 PM, Pavel Tikhomirov wrote:
>> v4: replace separate has pointer to CTL_FLAGS_HAS flag, second part in
>> patch "net/ipv4: add net_conf_op to reuse for ipv6"
>> v6: define CTL_FLAGS_HAS
>> v7: also allow EIO on do_sysctl_op for optional sysctls like
>> stable_secret and fix sysctl file to close in error path
>
> Wait a second. If we've managed to open a sysctl file, but failed to write into
> it, why should we pretend as if there was not this sysctl at all?

Now we only set CTL_FLAGS_OPTIONAL for op == CTL_READ. So it meant to 
be: if we opened sysctl but failed to read it, it is in 
not-yet-initialized state and we can skip it safely. Will adding an 
implicit op-check here be satisfactory?

>
>>
>> https://jira.sw.ru/browse/PSBM-30942
>>
>> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
>> ---
>>  criu/include/sysctl.h |  1 +
>>  criu/sysctl.c         | 21 +++++++++++++++++----
>>  2 files changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/criu/include/sysctl.h b/criu/include/sysctl.h
>> index b949a40..3611ef1 100644
>> --- a/criu/include/sysctl.h
>> +++ b/criu/include/sysctl.h
>> @@ -35,5 +35,6 @@ enum {
>>   * Some entries might be missing mark them as optional.
>>   */
>>  #define CTL_FLAGS_OPTIONAL	1
>> +#define CTL_FLAGS_HAS		2
>>
>>  #endif /* __CR_SYSCTL_H__ */
>> diff --git a/criu/sysctl.c b/criu/sysctl.c
>> index 21ae4ce..60e52db 100644
>> --- a/criu/sysctl.c
>> +++ b/criu/sysctl.c
>> @@ -303,8 +303,13 @@ static int __userns_sysctl_op(void *arg, int proc_fd, pid_t pid)
>>  		close(nsfd);
>>
>>  		for (i = 0; i < userns_req->nr_req; i++) {
>> -			if (do_sysctl_op(fds[i], reqs[i], op) < 0)
>> -				exit(1);
>> +			if (do_sysctl_op(fds[i], reqs[i], op) < 0) {
>> +				if (errno != EIO || !(req->flags & CTL_FLAGS_OPTIONAL))
>> +					exit(1);
>> +			} else {
>> +				/* mark sysctl in question exists */
>> +				req->flags |= CTL_FLAGS_HAS;
>> +			}
>>  		}
>>
>>  		exit(0);
>> @@ -372,8 +377,16 @@ static int __nonuserns_sysctl_op(struct sysctl_req *req, size_t nr_req, int op)
>>  		}
>>
>>  		ret = do_sysctl_op(fd, req, op);
>> -		if (ret)
>> -			goto out;
>> +		if (ret) {
>> +			if (errno != EIO || !(req->flags & CTL_FLAGS_OPTIONAL)) {
>> +				close(fd);
>> +				goto out;
>> +			}
>> +		} else {
>> +			/* mark sysctl in question exists */
>> +			req->flags |= CTL_FLAGS_HAS;
>> +		}
>> +
>>  		close(fd);
>>  		req++;
>>  	}
>>
>
Pavel Emelianov May 6, 2016, 12:37 p.m.
On 05/06/2016 03:10 PM, Pavel Tikhomirov wrote:
> On 05/06/2016 02:47 PM, Pavel Emelyanov wrote:
>> On 04/28/2016 07:38 PM, Pavel Tikhomirov wrote:
>>> v4: replace separate has pointer to CTL_FLAGS_HAS flag, second part in
>>> patch "net/ipv4: add net_conf_op to reuse for ipv6"
>>> v6: define CTL_FLAGS_HAS
>>> v7: also allow EIO on do_sysctl_op for optional sysctls like
>>> stable_secret and fix sysctl file to close in error path
>>
>> Wait a second. If we've managed to open a sysctl file, but failed to write into
>> it, why should we pretend as if there was not this sysctl at all?
> 
> Now we only set CTL_FLAGS_OPTIONAL for op == CTL_READ. So it meant to 
> be: if we opened sysctl but failed to read it, it is in 
> not-yet-initialized state and we can skip it safely.

I'm not sure that treating _any_ systl like this will work. Can you shed
more light on what's the issue with stable_secret sysctl is?

> Will adding an implicit op-check here be satisfactory?
> 
>>
>>>
>>> https://jira.sw.ru/browse/PSBM-30942
>>>
>>> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
>>> ---
>>>  criu/include/sysctl.h |  1 +
>>>  criu/sysctl.c         | 21 +++++++++++++++++----
>>>  2 files changed, 18 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/criu/include/sysctl.h b/criu/include/sysctl.h
>>> index b949a40..3611ef1 100644
>>> --- a/criu/include/sysctl.h
>>> +++ b/criu/include/sysctl.h
>>> @@ -35,5 +35,6 @@ enum {
>>>   * Some entries might be missing mark them as optional.
>>>   */
>>>  #define CTL_FLAGS_OPTIONAL	1
>>> +#define CTL_FLAGS_HAS		2
>>>
>>>  #endif /* __CR_SYSCTL_H__ */
>>> diff --git a/criu/sysctl.c b/criu/sysctl.c
>>> index 21ae4ce..60e52db 100644
>>> --- a/criu/sysctl.c
>>> +++ b/criu/sysctl.c
>>> @@ -303,8 +303,13 @@ static int __userns_sysctl_op(void *arg, int proc_fd, pid_t pid)
>>>  		close(nsfd);
>>>
>>>  		for (i = 0; i < userns_req->nr_req; i++) {
>>> -			if (do_sysctl_op(fds[i], reqs[i], op) < 0)
>>> -				exit(1);
>>> +			if (do_sysctl_op(fds[i], reqs[i], op) < 0) {
>>> +				if (errno != EIO || !(req->flags & CTL_FLAGS_OPTIONAL))
>>> +					exit(1);
>>> +			} else {
>>> +				/* mark sysctl in question exists */
>>> +				req->flags |= CTL_FLAGS_HAS;
>>> +			}
>>>  		}
>>>
>>>  		exit(0);
>>> @@ -372,8 +377,16 @@ static int __nonuserns_sysctl_op(struct sysctl_req *req, size_t nr_req, int op)
>>>  		}
>>>
>>>  		ret = do_sysctl_op(fd, req, op);
>>> -		if (ret)
>>> -			goto out;
>>> +		if (ret) {
>>> +			if (errno != EIO || !(req->flags & CTL_FLAGS_OPTIONAL)) {
>>> +				close(fd);
>>> +				goto out;
>>> +			}
>>> +		} else {
>>> +			/* mark sysctl in question exists */
>>> +			req->flags |= CTL_FLAGS_HAS;
>>> +		}
>>> +
>>>  		close(fd);
>>>  		req++;
>>>  	}
>>>
>>
> .
>
Pavel Tikhomirov May 6, 2016, 12:58 p.m.
On 05/06/2016 03:37 PM, Pavel Emelyanov wrote:
> On 05/06/2016 03:10 PM, Pavel Tikhomirov wrote:
>> On 05/06/2016 02:47 PM, Pavel Emelyanov wrote:
>>> On 04/28/2016 07:38 PM, Pavel Tikhomirov wrote:
>>>> v4: replace separate has pointer to CTL_FLAGS_HAS flag, second part in
>>>> patch "net/ipv4: add net_conf_op to reuse for ipv6"
>>>> v6: define CTL_FLAGS_HAS
>>>> v7: also allow EIO on do_sysctl_op for optional sysctls like
>>>> stable_secret and fix sysctl file to close in error path
>>>
>>> Wait a second. If we've managed to open a sysctl file, but failed to write into
>>> it, why should we pretend as if there was not this sysctl at all?
>>
>> Now we only set CTL_FLAGS_OPTIONAL for op == CTL_READ. So it meant to
>> be: if we opened sysctl but failed to read it, it is in
>> not-yet-initialized state and we can skip it safely.
>
> I'm not sure that treating _any_ systl like this will work. Can you shed
> more light on what's the issue with stable_secret sysctl is?

In addrconf_sysctl_stable_secret:

if (!write && !secret->initialized) {
         err = -EIO;
         goto out;
}

[root@dhcp-10-30-24-224 ~]# cat /proc/sys/net/ipv6/conf/lo/stable_secret
cat: /proc/sys/net/ipv6/conf/lo/stable_secret: Input/output error
[root@dhcp-10-30-24-224 ~]# echo 
"2607:f0d0:1002:0051:0000:0000:0000:0004" > 
/proc/sys/net/ipv6/conf/lo/stable_secret
[root@dhcp-10-30-24-224 ~]# cat /proc/sys/net/ipv6/conf/lo/stable_secret
2607:f0d0:1002:0051:0000:0000:0000:0004

So these sysctl should be first initialized and only after one can read it.

>
>> Will adding an implicit op-check here be satisfactory?
>>
>>>
>>>>
>>>> https://jira.sw.ru/browse/PSBM-30942
>>>>
>>>> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
>>>> ---
>>>>  criu/include/sysctl.h |  1 +
>>>>  criu/sysctl.c         | 21 +++++++++++++++++----
>>>>  2 files changed, 18 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/criu/include/sysctl.h b/criu/include/sysctl.h
>>>> index b949a40..3611ef1 100644
>>>> --- a/criu/include/sysctl.h
>>>> +++ b/criu/include/sysctl.h
>>>> @@ -35,5 +35,6 @@ enum {
>>>>   * Some entries might be missing mark them as optional.
>>>>   */
>>>>  #define CTL_FLAGS_OPTIONAL	1
>>>> +#define CTL_FLAGS_HAS		2
>>>>
>>>>  #endif /* __CR_SYSCTL_H__ */
>>>> diff --git a/criu/sysctl.c b/criu/sysctl.c
>>>> index 21ae4ce..60e52db 100644
>>>> --- a/criu/sysctl.c
>>>> +++ b/criu/sysctl.c
>>>> @@ -303,8 +303,13 @@ static int __userns_sysctl_op(void *arg, int proc_fd, pid_t pid)
>>>>  		close(nsfd);
>>>>
>>>>  		for (i = 0; i < userns_req->nr_req; i++) {
>>>> -			if (do_sysctl_op(fds[i], reqs[i], op) < 0)
>>>> -				exit(1);
>>>> +			if (do_sysctl_op(fds[i], reqs[i], op) < 0) {
>>>> +				if (errno != EIO || !(req->flags & CTL_FLAGS_OPTIONAL))
>>>> +					exit(1);
>>>> +			} else {
>>>> +				/* mark sysctl in question exists */
>>>> +				req->flags |= CTL_FLAGS_HAS;
>>>> +			}
>>>>  		}
>>>>
>>>>  		exit(0);
>>>> @@ -372,8 +377,16 @@ static int __nonuserns_sysctl_op(struct sysctl_req *req, size_t nr_req, int op)
>>>>  		}
>>>>
>>>>  		ret = do_sysctl_op(fd, req, op);
>>>> -		if (ret)
>>>> -			goto out;
>>>> +		if (ret) {
>>>> +			if (errno != EIO || !(req->flags & CTL_FLAGS_OPTIONAL)) {
>>>> +				close(fd);
>>>> +				goto out;
>>>> +			}
>>>> +		} else {
>>>> +			/* mark sysctl in question exists */
>>>> +			req->flags |= CTL_FLAGS_HAS;
>>>> +		}
>>>> +
>>>>  		close(fd);
>>>>  		req++;
>>>>  	}
>>>>
>>>
>> .
>>
>