* symlink handling - why the hell do we set non-NULL cookie for
symlinks that do not have any ->put_link()? What's worse, it looks like
a bogus server could trick us into overwriting ->link_target of a live
inode (e.g. via ->d_revalidate() -> pvfs2_inode_getattr() ->
copy_attributes_to_inode()). Am I misreading it?
* in copy_attributes_to_inode() we have
case PVFS_TYPE_SYMLINK:
if (symname != NULL) {
inode->i_size = (loff_t) strlen(symname);
break;
}
/*FALLTHRU*/
and the only caller passes is
if (copy_attributes_to_inode(inode,
&new_op->downcall.resp.getattr.attributes,
new_op->downcall.resp.getattr.link_target)) {
How can symname possibly be NULL? .resp.getattr.link_target is an array,
for crying out loud!
* who sets (or uses) struct PVFS_sys_attr_s .target_link? Or
.dist_name, or .dist_params, while we are at it...
* can a symlink be longer than 256 bytes? And why are
fs/orangefs/downcall.h:40: char link_target[PVFS2_NAME_LEN];
and
fs/orangefs/pvfs2-kernel.h:317: char link_target[PVFS_NAME_MAX];
spelled out differently? _Both_ constants are 256; what's the point of
obfuscating here?
* what's to guarantee that this .resp.getattr.link_target is
NUL-terminated? Passing it to strlen() in copy_attributes_to_inode()
is a bad idea unless we _have_ NUL in there...
* in the same copy_attributes_to_inode() you have
/* copy link target to inode private data */
if (pvfs2_inode && symname) {
strncpy(pvfs2_inode->link_target,
symname,
PVFS_NAME_MAX);
gossip_debug(GOSSIP_UTILS_DEBUG,
"Copied attr link target %s\n",
pvfs2_inode->link_target);
}
Again, what's to guarantee that ->link_target in pvfs2_inode will be
NUL-terminated?
* handling of ->i_mode in the same function is completely broken -
you are building it in place *ON* *LIVE* *INODE*:
inode->i_mode = perm_mode;
...
inode->i_mode |= S_IFDIR;
(and similar for regulars and symlinks). Again, that sucker is called from
your ->d_revalidate(). With no exclusion whatsoever, not that it would be
easy to provide one for all ->i_mode readers (it is possible, but you'd
have to replace a lot of generic helper functions with ones of your own;
not done and almost certainly not worth attempting).
* pvfs2_symlink() can't get NULL symname; what it *can* get is
symname up to 4Kb long. Doing
strncpy(new_op->upcall.req.sym.target, symname, PVFS2_NAME_LEN);
will not only quietly truncate it, it might leave the copy without NUL...
* what's up with compulsive casts? E.g.
attrs->mtime =
pvfs2_convert_time_field((void *)&iattr->ia_mtime);
with
__u64 pvfs2_convert_time_field(void *time_ptr)
{
__u64 pvfs2_time;
struct timespec *tspec = (struct timespec *)time_ptr;
pvfs2_time = (__u64) ((time_t) tspec->tv_sec);
return pvfs2_time;
}
Leaving aside the casts in (__u64) ((time_t) tspec->tv_sec), ->ia_mtime is
struct timespec; what's the point of taking its address, casting to void *,
passing to that sucker, only to cast to back to struct timespec * (and
only reading from it, so const struct timespec * would do just fine)?
* wrappers must die. It's not IOCCC, damn it. While having
pvfs2_inode_lock and pvfs2_lock_inode (both bogus) in the same header has
a certain charm, please don't do it.
* in dir.c:
for (i = 0; i < rhandle.readdir_response.pvfs_dirent_outcount; i++) {
len = rhandle.readdir_response.dirent_array[i].d_length;
current_entry = rhandle.readdir_response.dirent_array[i].d_name;
current_ino = pvfs2_khandle_to_ino(
&(rhandle.readdir_response.dirent_array[i].khandle));
gossip_debug(GOSSIP_DIR_DEBUG,
"calling dir_emit for %s with len %d, pos %ld\n",
current_entry,
len,
(unsigned long)pos);
ret =
dir_emit(ctx, current_entry, len, current_ino, DT_UNKNOWN);
ctx->pos++;
gossip_ldebug(GOSSIP_DIR_DEBUG,
"%s: ctx->pos:%lld\n",
__func__,
lld(ctx->pos));
pos++;
}
/* this means that all of the dir_emit calls succeeded */
if (i == rhandle.readdir_response.pvfs_dirent_outcount) {
No, it doesn't. You ignore the return value of dir_emit()...
* same file, decode_dirents():
readdir->dirent_array = kmalloc(readdir->pvfs_dirent_outcount *
sizeof(*readdir->dirent_array),
GFP_KERNEL);
What's to prevent a wraparound in multiplication here?
* same file, same function:
for (i = 0; i < readdir->pvfs_dirent_outcount; i++) {
dec_string(pptr, &readdir->dirent_array[i].d_name,
&readdir->dirent_array[i].d_length);
where
#define dec_string(pptr, pbuf, plen) do { \
__u32 len = (*(__u32 *) *(pptr)); \
*pbuf = *(pptr) + 4; \
*(pptr) += roundup8(4 + len + 1); \
if (plen) \
*plen = len;\
} while (0)
Just what will happen if this 32bit length will be well beyond
the size of response we are parsing here?
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
