Messages in this thread | | | Date | Fri, 24 Jan 2014 17:19:22 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH] dcache: error out if the name buffer is too short |
| |
On 01/24, Denys Vlasenko wrote: > > This change makes __dentry_path() and d_path() > immediately return ENAMETOOLONG if buflen < 2.
I am not sure about d_path, but as for __dentry_path:
> @@ -3122,13 +3125,14 @@ static char *__dentry_path(struct dentry *dentry, char *buf, int buflen) > int len, seq = 0; > int error = 0; > > + if (buflen < 2) > + goto Elong; > + > rcu_read_lock(); > restart: > end = buf + buflen; > len = buflen; > prepend(&end, &len, "\0", 1); > - if (buflen < 1) > - goto Elong;
you forgot to mention that this change fixes a bug, this "goto Elong" leaks rcu_read_lock().
And probably you are right, the fix should be as simple as possible. But can't we also simplify __dentry_path? Unless I missed something we can move prepend() up, before rcu_read_lock(), "move Get '/' right" into that prepend, and even kill retval... OK, most probably I missed something, but at first glance we can do something like
static char *__dentry_path(struct dentry *dentry, char *buf, int buflen) { int len, seq = 0; int error = 0; char *end;
buf += buflen; /* Get '/' right, write "/\0" at the end */ if (prepend(&buf, &buflen, "/", 2)) goto Elong;
rcu_read_lock(); restart: end = buf; len = buflen; read_seqbegin_or_lock(&rename_lock, &seq); while (!IS_ROOT(dentry)) { struct dentry *parent = dentry->d_parent; int error;
prefetch(parent); error = prepend_name(&end, &len, &dentry->d_name); if (error) break;
dentry = parent; } if (!(seq & 1)) rcu_read_unlock(); if (need_seqretry(&rename_lock, seq)) { seq = 1; goto restart; } done_seqretry(&rename_lock, seq); if (!error) return end; Elong: return ERR_PTR(-ENAMETOOLONG); }
Oleg.
| |