Re: [RFC/PATCH] revoke/frevoke system calls V2

[Edgar Toernig]

Pekka Enberg wrote:
>
> I'll put them on my todo and in the meanwhile, you can find the latest
> patches here:
>   http://www.kernel.org/pub/linux/kernel/people/penberg/patches/revoke/
> 
> Thanks for taking the time to review the patch!

+		err = close_files(this);
+
+		put_task_struct(this->owner);
+		if (err)
+			break;
+	}
+	if (err)
+		restore_files(&to_cleanup[i], nr_fds-i);

I think, the error path is wrong as it tries to restore "this"
which means the now invalid filp (close always closes, even in
case of errors) is put back into the fd-table; and, the task
struct is put twice.  I think, you should ignore errors on close.
(But I guess, this part of revoke gets rewritten anyway to match
BSD behaviour.)  I wonder, if revoke should really abort when
there's an error from one fd or better continue and try its best.

restore_files:
+			spin_lock(&files->file_lock);
+			fdt = files_fdtable(files);
+			rcu_assign_pointer(fdt->fd[this->fd], this->file);
+			FD_SET(this->fd, fdt->close_on_exec);
+			spin_unlock(&files->file_lock);
+			put_files_struct(files);
+		}
+
+		put_task_struct(this->owner);

This sets close_on_exec unconditionally, even if it wasn't set
before.  Hm..., if a cloned thread is able to exec, it seems a
little bit dangerous to restore the fd-table with filps that
were valid some time ago - the fd-table may have changed in the
meantime...  But maybe I simply missed something...

Ciao, ET.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/