diff mbox

[1/3] IB/core: Don't allow default GID addition at non-reserved slots

Message ID 20180412222427.24882-2-leon@kernel.org (mailing list archive)
State Superseded
Headers show

Commit Message

Leon Romanovsky April 12, 2018, 10:24 p.m. UTC 
From: Parav Pandit <parav@mellanox.com>

Default GIDs are marked reserved at the start of the GID table at index
0 and 1 by gid_table_reserve_default(). Currently when default GID is
requested, it can still allocates an empty slot which was not marked
as RESERVED for default GID which is incorrect.

At least in current code flow of roce_gid_mgmt.c likely in theory can
still request to allocate more than one/two default GIDs based on how
upper devices are setup. Therefore, it is better for cache layer
to allocate only reserved slot to default GID allocation request.

Fixes: 598ff6bae689 ("IB/core: Refactor GID modify code for RoCE")
Signed-off-by: Parav Pandit <parav@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/cache.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

--
2.14.3

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Bart Van Assche April 12, 2018, 10:33 p.m. UTC | #1 
On Fri, 2018-04-13 at 01:24 +0300, Leon Romanovsky wrote:
>  				/* Found an invalid (free) entry; allocate it */

> -				if (data->props & GID_TABLE_ENTRY_DEFAULT) {

> -					if (default_gid)

> +				if (default_gid) {

> +					/* If default GID is requested than, it

                                                                       ^^^^^
                                                                       then?

Additionally, please use the recommended comment style (start with /* on a line
by itself).

> +					 * should be one of the reserved entry.

> +					 * This ensures that only reserved slots

> +					 * are used for reserved GID.

> +					 */

> +					if (data->props & GID_TABLE_ENTRY_DEFAULT)

>  						empty = curr_index;

>  				} else {

>  					empty = curr_index;


Has it been considered to change the above nested if-statement into the following,
which is more compact?

				if (!default_gid || (data->props & GID_TABLE_ENTRY_DEFAULT))
					empty = curr_index

Thanks,

Bart.
Leon Romanovsky April 12, 2018, 10:52 p.m. UTC | #2 
On Thu, Apr 12, 2018 at 10:33:45PM +0000, Bart Van Assche wrote:
> On Fri, 2018-04-13 at 01:24 +0300, Leon Romanovsky wrote:
> >  				/* Found an invalid (free) entry; allocate it */
> > -				if (data->props & GID_TABLE_ENTRY_DEFAULT) {
> > -					if (default_gid)
> > +				if (default_gid) {
> > +					/* If default GID is requested than, it
>                                                                        ^^^^^
>                                                                        then?
>
> Additionally, please use the recommended comment style (start with /* on a line
> by itself).

Sure

>
> > +					 * should be one of the reserved entry.
> > +					 * This ensures that only reserved slots
> > +					 * are used for reserved GID.
> > +					 */
> > +					if (data->props & GID_TABLE_ENTRY_DEFAULT)
> >  						empty = curr_index;
> >  				} else {
> >  					empty = curr_index;
>
> Has it been considered to change the above nested if-statement into the following,
> which is more compact?
>
> 				if (!default_gid || (data->props & GID_TABLE_ENTRY_DEFAULT))
> 					empty = curr_index

Sending new version with compacted version of "if".

Thanks

>
> Thanks,
>
> Bart.
>
>
>
diff mbox

Patch

diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
index e337b08de2ff..8b2926da19e4 100644
--- a/drivers/infiniband/core/cache.c
+++ b/drivers/infiniband/core/cache.c
@@ -293,8 +293,13 @@  static int find_gid(struct ib_gid_table *table, const union ib_gid *gid,
 		if (pempty && empty < 0) {
 			if (data->props & GID_TABLE_ENTRY_INVALID) {
 				/* Found an invalid (free) entry; allocate it */
-				if (data->props & GID_TABLE_ENTRY_DEFAULT) {
-					if (default_gid)
+				if (default_gid) {
+					/* If default GID is requested than, it
+					 * should be one of the reserved entry.
+					 * This ensures that only reserved slots
+					 * are used for reserved GID.
+					 */
+					if (data->props & GID_TABLE_ENTRY_DEFAULT)
 						empty = curr_index;
 				} else {
 					empty = curr_index;