[PATCH] generic/484: Need another process to check record locks

[PATCH] generic/484: Need another process to check record locks

[Xiao Yang]

1) According to fcntl(2) manpage, A single process always gets F_UNLCK in
   the l_type field when using fcntl(F_GETLK) to acquire the existing lock
   set by itself because it could convert the existing lock to a new lock
   unconditionally.  So we need another process to check if the lock exists.

2) Remove redundant exit(0).

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 src/t_locks_execve.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/src/t_locks_execve.c b/src/t_locks_execve.c
index 9ad2dc3..d99d7de 100644
--- a/src/t_locks_execve.c
+++ b/src/t_locks_execve.c
@@ -8,6 +8,7 @@
 #include <errno.h>
 #include <pthread.h>
 #include <unistd.h>
+#include <sys/types.h>
 #include <sys/wait.h>
 
 static void err_exit(char *op, int errn)
@@ -32,12 +33,24 @@ struct flock fl = {
 
 static void checklock(int fd)
 {
-	if (fcntl(fd, F_GETLK, &fl) < 0)
-		err_exit("getlk", errno);
-	if (fl.l_type == F_UNLCK) {
-		printf("record lock is not preserved across execve(2)\n");
-		exit(1);
+	pid_t pid;
+
+	pid = fork();
+	if (pid < 0)
+		err_exit("fork", errno);
+
+	if (!pid) {
+		if (fcntl(fd, F_GETLK, &fl) < 0)
+			err_exit("getlk", errno);
+		if (fl.l_type == F_UNLCK) {
+			printf("record lock is not preserved across execve(2)\n");
+			exit(1);
+		}
+		exit(0);
 	}
+
+	waitpid(pid, NULL, 0);
+
 	exit(0);
 }
 
@@ -52,7 +65,6 @@ int main(int argc, char **argv)
 	if (argc == 3) {
 		fd = atoi(argv[2]);
 		checklock(fd);
-		exit(0);
 	}
 
 	fd = open(argv[1], O_WRONLY|O_CREAT, 0755);
-- 
1.8.3.1

Re: [PATCH] generic/484: Need another process to check record locks

[Xiong Murphy Zhou]

On Mon, May 21, 2018 at 01:42:00PM +0800, Xiao Yang wrote:
> 1) According to fcntl(2) manpage, A single process always gets F_UNLCK in
>    the l_type field when using fcntl(F_GETLK) to acquire the existing lock
>    set by itself because it could convert the existing lock to a new lock
>    unconditionally.  So we need another process to check if the lock exists.

I used to do that, eventaully I deleted it because we don't need
to check in another process in this case.

Thanks,
Xiong

> 
> 2) Remove redundant exit(0).
> 
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> ---
>  src/t_locks_execve.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/src/t_locks_execve.c b/src/t_locks_execve.c
> index 9ad2dc3..d99d7de 100644
> --- a/src/t_locks_execve.c
> +++ b/src/t_locks_execve.c
> @@ -8,6 +8,7 @@
>  #include <errno.h>
>  #include <pthread.h>
>  #include <unistd.h>
> +#include <sys/types.h>
>  #include <sys/wait.h>
>  
>  static void err_exit(char *op, int errn)
> @@ -32,12 +33,24 @@ struct flock fl = {
>  
>  static void checklock(int fd)
>  {
> -	if (fcntl(fd, F_GETLK, &fl) < 0)
> -		err_exit("getlk", errno);
> -	if (fl.l_type == F_UNLCK) {
> -		printf("record lock is not preserved across execve(2)\n");
> -		exit(1);
> +	pid_t pid;
> +
> +	pid = fork();
> +	if (pid < 0)
> +		err_exit("fork", errno);
> +
> +	if (!pid) {
> +		if (fcntl(fd, F_GETLK, &fl) < 0)
> +			err_exit("getlk", errno);
> +		if (fl.l_type == F_UNLCK) {
> +			printf("record lock is not preserved across execve(2)\n");
> +			exit(1);
> +		}
> +		exit(0);
>  	}
> +
> +	waitpid(pid, NULL, 0);
> +
>  	exit(0);
>  }
>  
> @@ -52,7 +65,6 @@ int main(int argc, char **argv)
>  	if (argc == 3) {
>  		fd = atoi(argv[2]);
>  		checklock(fd);
> -		exit(0);
>  	}
>  
>  	fd = open(argv[1], O_WRONLY|O_CREAT, 0755);
> -- 
> 1.8.3.1
> 
> 
> 

Re: [PATCH] generic/484: Need another process to check record locks

[Xiao Yang]

On 2018/05/22 10:36, Xiong Murphy Zhou wrote:
> On Mon, May 21, 2018 at 01:42:00PM +0800, Xiao Yang wrote:
>> 1) According to fcntl(2) manpage, A single process always gets F_UNLCK in
>>     the l_type field when using fcntl(F_GETLK) to acquire the existing lock
>>     set by itself because it could convert the existing lock to a new lock
>>     unconditionally.  So we need another process to check if the lock exists.
> I used to do that, eventaully I deleted it because we don't need
> to check in another process in this case.
Hi Xiong,

Even if the fix patch[1] has been applied, you always gets F_UNLCK without another process.

According to fcntl(2) manpage:
F_GETLK
	On  input  to this call, lock describes a lock we would like to place on the file.
	If the lock could be placed, fcntl() does not actually place it, but returns F_UNLCK
	in the l_type field of lock and leaves the other fields of the structure unchanged.

A single process can always place a new lock on the file and return F_UNLCK because the existing
lock is set by itself.

[1] https://www.spinics.net/lists/linux-fsdevel/msg123144.html

Thanks,
Xiao Yang

> Thanks,
> Xiong
>
>> 2) Remove redundant exit(0).
>>
>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
>> ---
>>   src/t_locks_execve.c | 24 ++++++++++++++++++------
>>   1 file changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/t_locks_execve.c b/src/t_locks_execve.c
>> index 9ad2dc3..d99d7de 100644
>> --- a/src/t_locks_execve.c
>> +++ b/src/t_locks_execve.c
>> @@ -8,6 +8,7 @@
>>   #include<errno.h>
>>   #include<pthread.h>
>>   #include<unistd.h>
>> +#include<sys/types.h>
>>   #include<sys/wait.h>
>>
>>   static void err_exit(char *op, int errn)
>> @@ -32,12 +33,24 @@ struct flock fl = {
>>
>>   static void checklock(int fd)
>>   {
>> -	if (fcntl(fd, F_GETLK,&fl)<  0)
>> -		err_exit("getlk", errno);
>> -	if (fl.l_type == F_UNLCK) {
>> -		printf("record lock is not preserved across execve(2)\n");
>> -		exit(1);
>> +	pid_t pid;
>> +
>> +	pid = fork();
>> +	if (pid<  0)
>> +		err_exit("fork", errno);
>> +
>> +	if (!pid) {
>> +		if (fcntl(fd, F_GETLK,&fl)<  0)
>> +			err_exit("getlk", errno);
>> +		if (fl.l_type == F_UNLCK) {
>> +			printf("record lock is not preserved across execve(2)\n");
>> +			exit(1);
>> +		}
>> +		exit(0);
>>   	}
>> +
>> +	waitpid(pid, NULL, 0);
>> +
>>   	exit(0);
>>   }
>>
>> @@ -52,7 +65,6 @@ int main(int argc, char **argv)
>>   	if (argc == 3) {
>>   		fd = atoi(argv[2]);
>>   		checklock(fd);
>> -		exit(0);
>>   	}
>>
>>   	fd = open(argv[1], O_WRONLY|O_CREAT, 0755);
>> -- 
>> 1.8.3.1
>>
>>
>>
>
> .
>

Re: [PATCH] generic/484: Need another process to check record locks

[Xiong Murphy Zhou]

On Tue, May 22, 2018 at 10:48:43AM +0800, Xiao Yang wrote:
> On 2018/05/22 10:36, Xiong Murphy Zhou wrote:
> > On Mon, May 21, 2018 at 01:42:00PM +0800, Xiao Yang wrote:
> > > 1) According to fcntl(2) manpage, A single process always gets F_UNLCK in
> > >     the l_type field when using fcntl(F_GETLK) to acquire the existing lock
> > >     set by itself because it could convert the existing lock to a new lock
> > >     unconditionally.  So we need another process to check if the lock exists.
> > I used to do that, eventaully I deleted it because we don't need
> > to check in another process in this case.
> Hi Xiong,
> 
> Even if the fix patch[1] has been applied, you always gets F_UNLCK without another process.
> 
> According to fcntl(2) manpage:
> F_GETLK
> 	On  input  to this call, lock describes a lock we would like to place on the file.
> 	If the lock could be placed, fcntl() does not actually place it, but returns F_UNLCK
> 	in the l_type field of lock and leaves the other fields of the structure unchanged.
> 
> A single process can always place a new lock on the file and return F_UNLCK because the existing
> lock is set by itself.

You are right. My bad. It was included in my initial post.

Ack for the patch.

Xiong

> 
> [1] https://www.spinics.net/lists/linux-fsdevel/msg123144.html
> 
> Thanks,
> Xiao Yang
> 
> > Thanks,
> > Xiong
> > 
> > > 2) Remove redundant exit(0).
> > > 
> > > Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
> > > ---
> > >   src/t_locks_execve.c | 24 ++++++++++++++++++------
> > >   1 file changed, 18 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/src/t_locks_execve.c b/src/t_locks_execve.c
> > > index 9ad2dc3..d99d7de 100644
> > > --- a/src/t_locks_execve.c
> > > +++ b/src/t_locks_execve.c
> > > @@ -8,6 +8,7 @@
> > >   #include<errno.h>
> > >   #include<pthread.h>
> > >   #include<unistd.h>
> > > +#include<sys/types.h>
> > >   #include<sys/wait.h>
> > > 
> > >   static void err_exit(char *op, int errn)
> > > @@ -32,12 +33,24 @@ struct flock fl = {
> > > 
> > >   static void checklock(int fd)
> > >   {
> > > -	if (fcntl(fd, F_GETLK,&fl)<  0)
> > > -		err_exit("getlk", errno);
> > > -	if (fl.l_type == F_UNLCK) {
> > > -		printf("record lock is not preserved across execve(2)\n");
> > > -		exit(1);
> > > +	pid_t pid;
> > > +
> > > +	pid = fork();
> > > +	if (pid<  0)
> > > +		err_exit("fork", errno);
> > > +
> > > +	if (!pid) {
> > > +		if (fcntl(fd, F_GETLK,&fl)<  0)
> > > +			err_exit("getlk", errno);
> > > +		if (fl.l_type == F_UNLCK) {
> > > +			printf("record lock is not preserved across execve(2)\n");
> > > +			exit(1);
> > > +		}
> > > +		exit(0);
> > >   	}
> > > +
> > > +	waitpid(pid, NULL, 0);
> > > +
> > >   	exit(0);
> > >   }
> > > 
> > > @@ -52,7 +65,6 @@ int main(int argc, char **argv)
> > >   	if (argc == 3) {
> > >   		fd = atoi(argv[2]);
> > >   		checklock(fd);
> > > -		exit(0);
> > >   	}
> > > 
> > >   	fd = open(argv[1], O_WRONLY|O_CREAT, 0755);
> > > -- 
> > > 1.8.3.1
> > > 
> > > 
> > > 
> > 
> > .
> > 
> 
> 
>