On Thu, Oct 22, 2015 at 01:41:58PM +0300, Dan Carpenter wrote:
> On Mon, Oct 19, 2015 at 10:11:29PM -0400, ira.we...@intel.com wrote:
> > + case HFI1_CMD_TID_INVAL_READ:
> > + ret = hfi1_user_exp_rcv_invalid(fp, &tinfo);
> > + if (!ret) {
> > + addr = (unsigned long)cmd.addr +
> > + offsetof(struct hfi1_tid_info, tidcnt);
> > + if (copy_to_user((void __user *)addr, &tinfo.tidcnt,
> > + sizeof(tinfo.tidcnt)))
> > + ret = -EFAULT;
> > + }
> > + break;
>
> This switch statement uses success handling throughtout instead of
> error handling. It's better if you write it like this:
>
> case HFI1_CMD_TID_INVAL_READ:
> ret = hfi1_user_exp_rcv_invalid(fp, &tinfo);
> if (ret)
> break;
>
> addr = (unsigned long)cmd.addr +
> offsetof(struct hfi1_tid_info, tidcnt);
> if (copy_to_user((void __user *)addr, &tinfo.tidcnt,
> sizeof(tinfo.tidcnt)))
> ret = -EFAULT;
> break;
This follows the rest of the style of the case statement in this function. We
prefer to leave this as is for a number of reasons.
1) This is consistent with the coding style elsewhere in this driver.
2) It is functionally equivalent.
3) I have a long list of patches which need to be processed and this may cause
later merge conflicts.
The main reason is number 1. We want to remain consistent within this driver.
>
>
> The casting is sort of ugly... It would be better to make address a
> pointer. It does cut down on the lines of code but at least the cast is
> all done at once and really "addr" is actually a pointer.
>
> case HFI1_CMD_TID_INVAL_READ:
> ret = hfi1_user_exp_rcv_invalid(fp, &tinfo);
> if (ret)
> break;
>
> addr = (void __user *)(unsigned long)cmd.addr +
> offsetof(struct hfi1_tid_info, tidcnt);
> if (copy_to_user(addr, &tinfo.tidcnt, sizeof(tinfo.tidcnt)))
> ret = -EFAULT;
> break;
I think this is more a matter of style.
>
>
> > case HFI1_CMD_TID_FREE:
> > - ret = exp_tid_free(fp, &tinfo);
> > + ret = hfi1_user_exp_rcv_clear(fp, &tinfo);
> > + addr = (unsigned long)cmd.addr +
> > + offsetof(struct hfi1_tid_info, tidcnt);
> > + if (copy_to_user((void __user *)addr, &tinfo.tidcnt,
> > + sizeof(tinfo.tidcnt)))
> > + ret = -EFAULT;
> > break;
>
> This is an information leak. We should break if
> hfi1_user_exp_rcv_clear() fails, but instead we copy uninitialized
> variables to the user.
That is a bug, thanks.
Fixed in v3 (Although, I did use the same positive error check style to be
consistent within this function.)
>
>
>
> > case HFI1_CMD_RECV_CTRL:
> > ret = manage_rcvq(uctxt, subctxt_fp(fp), (int)user_val);
> > @@ -607,9 +603,9 @@ static int hfi1_file_mmap(struct file *fp, struct
> > vm_area_struct *vma)
> > * Use the page where this context's flags are. User level
> > * knows where it's own bitmap is within the page.
> > */
> > - memaddr = ((unsigned long)dd->events +
> > - ((uctxt->ctxt - dd->first_user_ctxt) *
> > - HFI1_MAX_SHARED_CTXTS)) & PAGE_MASK;
> > + memaddr = (unsigned long)(dd->events +
> > + ((uctxt->ctxt - dd->first_user_ctxt) *
> > + HFI1_MAX_SHARED_CTXTS)) & PAGE_MASK;
>
> I am too lazy to investigate the types of all these variables but I'm
> instead going to assert that moving the cast to later is an unrelated
> white space change. Don't mix white space changes in with a behavior
> change patch because it makes it hard to review.
Actually this is a bug fix. dd->events is a pointer which needs to be indexed
prior to the cast. I'll split this into a separate patch in v3.
Thanks!
Ira
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
