Re: [PATCH rdma-next 07/14] RDMA/uverbs: Refactor command header processing

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2398 bytes --]

On Wed, Feb 14, 2018 at 04:49:51PM -0700, Jason Gunthorpe wrote:
> On Wed, Feb 14, 2018 at 02:38:37PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > Move all command header processing into separate function
> > and perform those checks before acquiring SRCU read lock.
> >
> > Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >  drivers/infiniband/core/uverbs_main.c | 62 ++++++++++++++++++-----------------
> >  1 file changed, 32 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> > index 90e0b16aed1a..50d43c300112 100644
> > +++ b/drivers/infiniband/core/uverbs_main.c
> > @@ -660,6 +660,29 @@ static bool verify_command_idx(__u32 command, bool extended)
> >  	       uverbs_cmd_table[command];
> >  }
> >
> > +static ssize_t process_hdr(struct ib_uverbs_cmd_hdr *hdr,
> > +			   __u32 *command, bool *extended)
> > +{
> > +	__u32 flags;
> > +
> > +	if (hdr->command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
> > +				   IB_USER_VERBS_CMD_COMMAND_MASK))
> > +		return -EINVAL;
> > +
> > +	*command = hdr->command & IB_USER_VERBS_CMD_COMMAND_MASK;
> > +	flags = (hdr->command &
> > +		 IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT;
> > +
> > +	*extended = flags & IB_USER_VERBS_CMD_FLAG_EXTENDED;
> > +	if (flags & ~IB_USER_VERBS_CMD_FLAG_EXTENDED)
> > +		return -EINVAL;
>
> er..
>
> We don't need both
>
> 	if (hdr->command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
> 				   IB_USER_VERBS_CMD_COMMAND_MASK))
>
> and
>
>         if (flags & ~IB_USER_VERBS_CMD_FLAG_EXTENDED)
> 		return -EINVAL;
>
> They test the same condition.

Not exactly,
~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK | IB_USER_VERBS_CMD_COMMAND_MASK)
== ~(__u32)(0xff000000u| 0xff) == 0x00ffff00

=> we are testing that middle bits of hdr->command are cleared

flags & ~IB_USER_VERBS_CMD_FLAG_EXTENDED == ((hdr->command &
IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT) &
~IB_USER_VERBS_CMD_FLAG_EXTENDED == 0xff000000u >> 24 & ~0x80 ==
0xff & 0x7F == 0x7F

=> we are testing that only extended flag is enable.

>
> So my earlier comment about needing a stronger flags check is bogus -
> we already have the stronger check. The original code was just
> checking it twice, so it doesn't need this extra if.
>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]