On Thu, Feb 15, 2018 at 03:51:20PM +0200, Leon Romanovsky wrote:
> 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@xxxxxxxxxxxx>
> > >
> > > Move all command header processing into separate function
> > > and perform those checks before acquiring SRCU read lock.
> > >
> > > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > 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.
Well, I ment it should have just been:
if (hdr->command & ~(u32)(IB_USER_VERBS_CMD_FLAG_EXTENDED |
IB_USER_VERBS_CMD_COMMAND_MASK))
Then we don't need two ifs
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
