[PATCH 0/1] exec: do not sleep in TASK_TRACED under ->cred_guard_mutex

[PATCH 0/1] exec: do not sleep in TASK_TRACED under ->cred_guard_mutex

[Oleg Nesterov]

I cc'ed Linus and stable, but I don't know how to really test this patch
and I don't really understand the new creds management.

Please review.


load_flat_shared_library() does something strange (but hopefully this
patch doesn't break it). I do not understand why does it create the
new bprm. Afaics, it could reuse bprm pointer which comes as an argument
of ->load_binary(), all we need is to temporary change/restore bprm->file
for load_flat_file().

Oleg.

binfmt_flat.c && bprm->cred (Was: [PATCH 0/1] exec: do not sleep in TASK_TRACED under ->cred_guard_mutex)

[Oleg Nesterov]

On 09/03, Oleg Nesterov wrote:
>
> load_flat_shared_library() does something strange (but hopefully this
> patch doesn't break it). I do not understand why does it create the
> new bprm. Afaics, it could reuse bprm pointer which comes as an argument
> of ->load_binary(), all we need is to temporary change/restore bprm->file
> for load_flat_file().

IOW, afaics the patch below makes sense. Imho it is a bit ugly binfmt_flat.c
plays with prepare_exec_creds().

But again, I don't understand this code, and I didn't even try to compile
this patch.

Oleg.

--- a/fs/binfmt_flat.c
+++ b/fs/binfmt_flat.c
@@ -83,7 +83,7 @@ struct lib_info {
 };
 
 #ifdef CONFIG_BINFMT_SHARED_FLAT
-static int load_flat_shared_library(int id, struct lib_info *p);
+static int load_flat_shared_library(struct linux_binprm*, int, struct lib_info*);
 #endif
 
 static int load_flat_binary(struct linux_binprm *, struct pt_regs * regs);
@@ -308,7 +308,8 @@ out_free:
 /****************************************************************************/
 
 static unsigned long
-calc_reloc(unsigned long r, struct lib_info *p, int curid, int internalp)
+calc_reloc(struct linux_binprm *bprm, unnsigned long r, struct lib_info *p,
+		int curid, int internalp)
 {
 	unsigned long addr;
 	int id;
@@ -335,7 +336,7 @@ calc_reloc(unsigned long r, struct lib_i
 					"(%d != %d)", (unsigned) r, curid, id);
 			goto failed;
 		} else if ( ! p->lib_list[id].loaded &&
-				load_flat_shared_library(id, p) > (unsigned long) -4096) {
+				load_flat_shared_library(bprm, id, p) > (unsigned long) -4096) {
 			printk("BINFMT_FLAT: failed to load library %d", id);
 			goto failed;
 		}
@@ -726,7 +727,7 @@ static int load_flat_file(struct linux_b
 		for (rp = (unsigned long *)datapos; *rp != 0xffffffff; rp++) {
 			unsigned long addr;
 			if (*rp) {
-				addr = calc_reloc(*rp, libinfo, id, 0);
+				addr = calc_reloc(bprm, *rp, libinfo, id, 0);
 				if (addr == RELOC_FAILED) {
 					ret = -ENOEXEC;
 					goto err;
@@ -759,7 +760,7 @@ static int load_flat_file(struct linux_b
 			if (flat_set_persistent (relval, &persistent))
 				continue;
 			addr = flat_get_relocate_addr(relval);
-			rp = (unsigned long *) calc_reloc(addr, libinfo, id, 1);
+			rp = (unsigned long *) calc_reloc(bprm, addr, libinfo, id, 1);
 			if (rp == (unsigned long *)RELOC_FAILED) {
 				ret = -ENOEXEC;
 				goto err;
@@ -775,7 +776,7 @@ static int load_flat_file(struct linux_b
 				 */
 				if ((flags & FLAT_FLAG_GOTPIC) == 0)
 					addr = ntohl(addr);
-				addr = calc_reloc(addr, libinfo, id, 0);
+				addr = calc_reloc(bprm, addr, libinfo, id, 0);
 				if (addr == RELOC_FAILED) {
 					ret = -ENOEXEC;
 					goto err;
@@ -812,37 +813,29 @@ err:
  * segment (including bss) but not argv/argc/environ.
  */
 
-static int load_flat_shared_library(int id, struct lib_info *libs)
+static int load_flat_shared_library(struct linux_binprm *bprm, int id,
+						struct lib_info *libs)
 {
-	struct linux_binprm bprm;
-	int res;
+	char *save_name = bprm->filename;
+	struct file *save_file = bprm->file;
 	char buf[16];
-
-	/* Create the file name */
-	sprintf(buf, "/lib/lib%d.so", id);
+	int res;
 
 	/* Open the file up */
-	bprm.filename = buf;
-	bprm.file = open_exec(bprm.filename);
-	res = PTR_ERR(bprm.file);
-	if (IS_ERR(bprm.file))
-		return res;
-
-	bprm.cred = prepare_exec_creds();
-	res = -ENOMEM;
-	if (!bprm.cred)
-		goto out;
-
-	res = prepare_binprm(&bprm);
-
-	if (res <= (unsigned long)-4096)
-		res = load_flat_file(&bprm, libs, id, NULL);
-
-	abort_creds(bprm.cred);
-
-out:
-	allow_write_access(bprm.file);
-	fput(bprm.file);
+	sprintf(buf, "/lib/lib%d.so", id);
+	bprm->filename = buf;
+	bprm->file = open_exec(bprm->filename);
+	res = PTR_ERR(bprm->file);
+	if (IS_ERR(bprm->file))
+		goto ret;
+
+	res = load_flat_file(bprm, libs, id, NULL);
+
+	allow_write_access(bprm->file);
+	fput(bprm->file);
+ret:
+	bprm->filename = save_name;
+	bprm->file = save_file;
 
 	return(res);
 }

Re: binfmt_flat.c && bprm->cred (Was: [PATCH 0/1] exec: do not sleep in TASK_TRACED under ->cred_guard_mutex)

[Cyrill Gorcunov]

[Oleg Nesterov - Thu, Sep 03, 2009 at 06:29:39PM +0200]
| On 09/03, Oleg Nesterov wrote:
| >
| > load_flat_shared_library() does something strange (but hopefully this
| > patch doesn't break it). I do not understand why does it create the
| > new bprm. Afaics, it could reuse bprm pointer which comes as an argument
| > of ->load_binary(), all we need is to temporary change/restore bprm->file
| > for load_flat_file().
| 
| IOW, afaics the patch below makes sense. Imho it is a bit ugly binfmt_flat.c
| plays with prepare_exec_creds().
| 
| But again, I don't understand this code, and I didn't even try to compile
| this patch.
| 
| Oleg.
| 
...  
| -static int load_flat_shared_library(int id, struct lib_info *libs)
| +static int load_flat_shared_library(struct linux_binprm *bprm, int id,
| +						struct lib_info *libs)
|  {
...
| +	sprintf(buf, "/lib/lib%d.so", id);

Hi Oleg, perhaps it is a good moment to switch sprintf to snprintf
as well? buf is only 16 bytes long so we have 4 byte room for number.
Not sure if it's possible to have 10000 relocs though :) Just a thought.
Most probably I miss something.

	-- Cyrill

Re: binfmt_flat.c && bprm->cred (Was: [PATCH 0/1] exec: do not sleep in TASK_TRACED under ->cred_guard_mutex)

[Cyrill Gorcunov]

[Cyrill Gorcunov - Thu, Sep 03, 2009 at 08:58:50PM +0400]
...
| ...  
| | -static int load_flat_shared_library(int id, struct lib_info *libs)
| | +static int load_flat_shared_library(struct linux_binprm *bprm, int id,
| | +						struct lib_info *libs)
| |  {
| ...
| | +	sprintf(buf, "/lib/lib%d.so", id);
| 
| Hi Oleg, perhaps it is a good moment to switch sprintf to snprintf
| as well? buf is only 16 bytes long so we have 4 byte room for number.
| Not sure if it's possible to have 10000 relocs though :) Just a thought.
| Most probably I miss something.
| 
| 	-- Cyrill

Sigh... I'm idiot. We have MAX_SHARED_LIBS limit here. Drop my question
please. Sorry for noise.

	-- Cyrill

Re: binfmt_flat.c && bprm->cred (Was: [PATCH 0/1] exec: do not sleep in TASK_TRACED under ->cred_guard_mutex)

[Oleg Nesterov]

On 09/03, Cyrill Gorcunov wrote:
>
> [Oleg Nesterov - Thu, Sep 03, 2009 at 06:29:39PM +0200]
> | On 09/03, Oleg Nesterov wrote:
> | >
> | > load_flat_shared_library() does something strange (but hopefully this
> | > patch doesn't break it). I do not understand why does it create the
> | > new bprm. Afaics, it could reuse bprm pointer which comes as an argument
> | > of ->load_binary(), all we need is to temporary change/restore bprm->file
> | > for load_flat_file().
> |
> | IOW, afaics the patch below makes sense. Imho it is a bit ugly binfmt_flat.c
> | plays with prepare_exec_creds().
> |
> | But again, I don't understand this code, and I didn't even try to compile
> | this patch.
> |
> | Oleg.
> |
> ...
> | -static int load_flat_shared_library(int id, struct lib_info *libs)
> | +static int load_flat_shared_library(struct linux_binprm *bprm, int id,
> | +						struct lib_info *libs)
> |  {
> ...
> | +	sprintf(buf, "/lib/lib%d.so", id);
>
> Hi Oleg, perhaps it is a good moment to switch sprintf to snprintf
> as well? buf is only 16 bytes long so we have 4 byte room for number.

Agreed. As you pointed out privately we have MAX_SHARED_LIBS=4, but
still snprintf() is safer.

Oleg.

Re: [PATCH 0/1] exec: do not sleep in TASK_TRACED under ->cred_guard_mutex

[David Howells]

Oleg Nesterov <oleg@redhat.com> wrote:

> [PATCH 0/1] exec: do not sleep in TASK_TRACED under ->cred_guard_mutex

I don't see where it is sleeping in TASK_TRACED with cred_guard_mutex held.

David

Re: [PATCH 0/1] exec: do not sleep in TASK_TRACED under ->cred_guard_mutex

[Roland McGrath]

> Oleg Nesterov <oleg@redhat.com> wrote:
> 
> > [PATCH 0/1] exec: do not sleep in TASK_TRACED under ->cred_guard_mutex
> 
> I don't see where it is sleeping in TASK_TRACED with cred_guard_mutex held.

search_binary_handler calls tracehook_report_exec, which calls
ptrace_notify when PTRACE_O_TRACEEXEC is enabled.


Thanks,
Roland

Re: [PATCH 0/1] exec: do not sleep in TASK_TRACED under ->cred_guard_mutex

[David Howells]

Roland McGrath <roland@redhat.com> wrote:

> search_binary_handler calls tracehook_report_exec, which calls
> ptrace_notify when PTRACE_O_TRACEEXEC is enabled.

Ah.  Now I see.

David