[4/6] kdat: Make sure we're readin cache from tmpfs

Submitted by Cyrill Gorcunov on April 3, 2018, 5:46 p.m.

Details

Message ID 20180403174650.30628-5-gorcunov@gmail.com
State Rejected
Series "Preparatory patches for handling duped tfd on epoll"
Headers show

Commit Message

Cyrill Gorcunov April 3, 2018, 5:46 p.m.
When we're saving kdat we check if fs is laying on
is on tmpfs thus on machine reboot it get removed
and regenerated on next run. Still there is no
proof that when we're reading it the underlied
fs has not been changed. Lets add this check.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 criu/kerndat.c | 7 +++++++
 1 file changed, 7 insertions(+)

Patch hide | download patch | download mbox

diff --git a/criu/kerndat.c b/criu/kerndat.c
index 798cb16274e4..227d429f9dde 100644
--- a/criu/kerndat.c
+++ b/criu/kerndat.c
@@ -840,6 +840,7 @@  static int kerndat_x86_has_ptrace_fpu_xsave_bug(void)
 
 static int kerndat_try_load_cache(void)
 {
+	struct statfs s;
 	int fd, ret;
 
 	fd = open(KERNDAT_CACHE_FILE, O_RDONLY);
@@ -855,6 +856,12 @@  static int kerndat_try_load_cache(void)
 		return -1;
 	}
 
+	if (fstatfs(fd, &s) < 0 || s.f_type != TMPFS_MAGIC) {
+		pr_warn("Can't read kdat cache from non-tempfs\n");
+		close(fd);
+		return -1;
+	}
+
 	close(fd);
 
 	if (ret != sizeof(kdat) ||

Comments

Dmitry Safonov April 3, 2018, 6:05 p.m.
2018-04-03 18:46 GMT+01:00 Cyrill Gorcunov <gorcunov@gmail.com>:
> When we're saving kdat we check if fs is laying on
> is on tmpfs thus on machine reboot it get removed
> and regenerated on next run. Still there is no
> proof that when we're reading it the underlied
> fs has not been changed. Lets add this check.
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

I'm not really sure if we need to care about someone remounting fs
and placing a copy of criu.kdat over again..
I don't mind, maybe we should bother, I don't have an opinion about this.

Anyway, +Cc: Pasha, Andrei, who also introduced this.
Cyrill Gorcunov April 4, 2018, 8:21 a.m.
On Tue, Apr 03, 2018 at 07:05:26PM +0100, Dmitry Safonov wrote:
> 2018-04-03 18:46 GMT+01:00 Cyrill Gorcunov <gorcunov@gmail.com>:
> > When we're saving kdat we check if fs is laying on
> > is on tmpfs thus on machine reboot it get removed
> > and regenerated on next run. Still there is no
> > proof that when we're reading it the underlied
> > fs has not been changed. Lets add this check.
> >
> > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> 
> I'm not really sure if we need to care about someone remounting fs
> and placing a copy of criu.kdat over again..
> I don't mind, maybe we should bother, I don't have an opinion about this.
> 
> Anyway, +Cc: Pasha, Andrei, who also introduced this.

Thanks a lot for review, Dima! I'll resend the series one day.
Pavel Emelyanov April 5, 2018, 7:57 a.m.
On 04/03/2018 08:46 PM, Cyrill Gorcunov wrote:
> When we're saving kdat we check if fs is laying on
> is on tmpfs thus on machine reboot it get removed
> and regenerated on next run. Still there is no
> proof that when we're reading it the underlied
> fs has not been changed. Lets add this check.

Can you describe the scenario in which kdat.cache writing happened on tmpfs, while
reading on non-tmpfs?

> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  criu/kerndat.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/criu/kerndat.c b/criu/kerndat.c
> index 798cb16274e4..227d429f9dde 100644
> --- a/criu/kerndat.c
> +++ b/criu/kerndat.c
> @@ -840,6 +840,7 @@ static int kerndat_x86_has_ptrace_fpu_xsave_bug(void)
>  
>  static int kerndat_try_load_cache(void)
>  {
> +	struct statfs s;
>  	int fd, ret;
>  
>  	fd = open(KERNDAT_CACHE_FILE, O_RDONLY);
> @@ -855,6 +856,12 @@ static int kerndat_try_load_cache(void)
>  		return -1;
>  	}
>  
> +	if (fstatfs(fd, &s) < 0 || s.f_type != TMPFS_MAGIC) {
> +		pr_warn("Can't read kdat cache from non-tempfs\n");
> +		close(fd);
> +		return -1;
> +	}
> +
>  	close(fd);
>  
>  	if (ret != sizeof(kdat) ||
>
Cyrill Gorcunov April 5, 2018, 8:17 a.m.
On Thu, Apr 05, 2018 at 10:57:57AM +0300, Pavel Emelyanov wrote:
> On 04/03/2018 08:46 PM, Cyrill Gorcunov wrote:
> > When we're saving kdat we check if fs is laying on
> > is on tmpfs thus on machine reboot it get removed
> > and regenerated on next run. Still there is no
> > proof that when we're reading it the underlied
> > fs has not been changed. Lets add this check.
> 
> Can you describe the scenario in which kdat.cache writing happened on tmpfs, while
> reading on non-tmpfs?

I thought I put it into changelog. The fs happen to change for various reasons
(including node operator errors), the check has neglible perf penalty so I think
it worth having it here. The patch series has to be reworked anyway so for
now just drop it.