Re: [GIT PULL] Orangefs (text only resend)

[Al Viro <viro@ZenIV.linux.org.uk>]

	* 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html