[29/37] lustre: handles: discard h_owner in favour of h_ops
diff mbox series

Message ID 155053494646.24125.10155576042684274174.stgit@noble.brown
State New
Headers show
Series
  • More lustre patches from obdclass
Related show

Commit Message

NeilBrown Feb. 19, 2019, 12:09 a.m. UTC
lustre_handles  assigned a  64bit  unique identifier  (a 'cookie')  to
objects of  various types and  stored them  in a hash  table, allowing
them to be accessed by the cookie.

The is a facility for type checking by recording an 'owner' for each
object, and checking the owner on lookup.  Unfortunately this is not
used - owner is always zero.

Eahc object also contains an h_ops pointer which can be used to
reliably identify an owner.

So discard h_owner, pass and 'ops' pointer to class_handle2object(),
and only return objects for which the h_ops matches.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 .../staging/lustre/lustre/include/lustre_handles.h |    7 +++----
 drivers/staging/lustre/lustre/ldlm/ldlm_lock.c     |    2 +-
 drivers/staging/lustre/lustre/obdclass/genops.c    |    3 ++-
 .../lustre/lustre/obdclass/lustre_handles.c        |    6 +++---
 4 files changed, 9 insertions(+), 9 deletions(-)

Comments

Andreas Dilger Feb. 27, 2019, 6:37 a.m. UTC | #1
On Feb 18, 2019, at 17:09, NeilBrown <neilb@suse.com> wrote:
> 
> lustre_handles  assigned a  64bit  unique identifier  (a 'cookie')  to
> objects of  various types and  stored them  in a hash  table, allowing
> them to be accessed by the cookie.
> 
> The is a facility for type checking by recording an 'owner' for each
> object, and checking the owner on lookup.  Unfortunately this is not
> used - owner is always zero.
> 
> Eahc object also contains an h_ops pointer which can be used to
> reliably identify an owner.
> 
> So discard h_owner, pass and 'ops' pointer to class_handle2object(),
> and only return objects for which the h_ops matches.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>

The h_owner is not used in the client code, but it is still used in the
server code to ensure that only clients which have already opened a file
handle can use the open file handle cookie to re-open the file.  The
h_owner in this case is an MDS-local pointer to the client export data,
so just using the h_ops pointer (which is generic to the class of users,
not the specific user) would not provide this functionality.

Cheers, Andreas
---
Andreas Dilger
Principal Lustre Architect
Whamcloud
NeilBrown Feb. 27, 2019, 9:41 p.m. UTC | #2
On Wed, Feb 27 2019, Andreas Dilger wrote:

> On Feb 18, 2019, at 17:09, NeilBrown <neilb@suse.com> wrote:
>> 
>> lustre_handles  assigned a  64bit  unique identifier  (a 'cookie')  to
>> objects of  various types and  stored them  in a hash  table, allowing
>> them to be accessed by the cookie.
>> 
>> The is a facility for type checking by recording an 'owner' for each
>> object, and checking the owner on lookup.  Unfortunately this is not
>> used - owner is always zero.
>> 
>> Eahc object also contains an h_ops pointer which can be used to
>> reliably identify an owner.
>> 
>> So discard h_owner, pass and 'ops' pointer to class_handle2object(),
>> and only return objects for which the h_ops matches.
>> 
>> Signed-off-by: NeilBrown <neilb@suse.com>
>
> The h_owner is not used in the client code, but it is still used in the
> server code to ensure that only clients which have already opened a file
> handle can use the open file handle cookie to re-open the file.  The
> h_owner in this case is an MDS-local pointer to the client export data,
> so just using the h_ops pointer (which is generic to the class of users,
> not the specific user) would not provide this functionality.

Is it possible for two different clients to be using the same cookie?
The lookup code in class_handle2object() appears to allow this, but I
assume it doesn't happen.

If it *does*, then the 'owner' does need to be part of the lookup key,
at least on the server.
If it doesn't, then I think it would be better for the owner to live in
struct mdt_file_data rather than in the handle.  In the handle it
wastes space in the thousands of instances that don't need it.

Thanks,
NeilBrown

>
> Cheers, Andreas
> ---
> Andreas Dilger
> Principal Lustre Architect
> Whamcloud
Andreas Dilger Feb. 28, 2019, 6:41 a.m. UTC | #3
On Feb 27, 2019, at 14:41, NeilBrown <neilb@suse.com> wrote:
> 
> On Wed, Feb 27 2019, Andreas Dilger wrote:
> 
>> On Feb 18, 2019, at 17:09, NeilBrown <neilb@suse.com> wrote:
>>> 
>>> lustre_handles  assigned a  64bit  unique identifier  (a 'cookie')  to
>>> objects of  various types and  stored them  in a hash  table, allowing
>>> them to be accessed by the cookie.
>>> 
>>> The is a facility for type checking by recording an 'owner' for each
>>> object, and checking the owner on lookup.  Unfortunately this is not
>>> used - owner is always zero.
>>> 
>>> Eahc object also contains an h_ops pointer which can be used to
>>> reliably identify an owner.
>>> 
>>> So discard h_owner, pass and 'ops' pointer to class_handle2object(),
>>> and only return objects for which the h_ops matches.
>>> 
>>> Signed-off-by: NeilBrown <neilb@suse.com>
>> 
>> The h_owner is not used in the client code, but it is still used in the
>> server code to ensure that only clients which have already opened a file
>> handle can use the open file handle cookie to re-open the file.  The
>> h_owner in this case is an MDS-local pointer to the client export data,
>> so just using the h_ops pointer (which is generic to the class of users,
>> not the specific user) would not provide this functionality.
> 
> Is it possible for two different clients to be using the same cookie?
> The lookup code in class_handle2object() appears to allow this, but I
> assume it doesn't happen.
> 
> If it *does*, then the 'owner' does need to be part of the lookup key,
> at least on the server.
> If it doesn't, then I think it would be better for the owner to live in
> struct mdt_file_data rather than in the handle.  In the handle it
> wastes space in the thousands of instances that don't need it.

No, since the cookie is generated by the MDS in this case, it should be
unique among all clients, and a single open file handle can't be used
by multiple clients.  The goal of class_handle2object() was to ensure
that the handle was only valid for the right caller, and some other
client couldn't accidentally or maliciously use the same cookie to get
access to an object that they weren't allowed to.

That said, it looks like the caller mdt_open_handle2mfd() could also do
this check itself, to verify that the found handle actually belongs to
the client using it.  That would need an "owner" pointer to be stored in
struct mdt_file_data to reference the export, but at least this overhead
would be contained to the one place that needs it.

While the MDS could also verify this by scanning the entire list of open
files for that client, there could potentially be thousands of files per
client that need to be checked.

Cheers, Andreas
---
Andreas Dilger
Principal Lustre Architect
Whamcloud

Patch
diff mbox series

diff --git a/drivers/staging/lustre/lustre/include/lustre_handles.h b/drivers/staging/lustre/lustre/include/lustre_handles.h
index 683680891e4c..9a4b1a821e7b 100644
--- a/drivers/staging/lustre/lustre/include/lustre_handles.h
+++ b/drivers/staging/lustre/lustre/include/lustre_handles.h
@@ -65,8 +65,7 @@  struct portals_handle_ops {
 struct portals_handle {
 	struct list_head		h_link;
 	u64				h_cookie;
-	const void			*h_owner;
-	struct portals_handle_ops	*h_ops;
+	const struct portals_handle_ops	*h_ops;
 
 	/* newly added fields to handle the RCU issue. -jxiong */
 	struct rcu_head			h_rcu;
@@ -79,9 +78,9 @@  struct portals_handle {
 
 /* Add a handle to the hash table */
 void class_handle_hash(struct portals_handle *,
-		       struct portals_handle_ops *ops);
+		       const struct portals_handle_ops *ops);
 void class_handle_unhash(struct portals_handle *);
-void *class_handle2object(u64 cookie, const void *owner);
+void *class_handle2object(u64 cookie, const struct portals_handle_ops *ops);
 void class_handle_free_cb(struct rcu_head *rcu);
 int class_handle_init(void);
 void class_handle_cleanup(void);
diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_lock.c b/drivers/staging/lustre/lustre/ldlm/ldlm_lock.c
index 60a6ec2f7136..248331153c68 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_lock.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_lock.c
@@ -503,7 +503,7 @@  struct ldlm_lock *__ldlm_handle2lock(const struct lustre_handle *handle,
 
 	LASSERT(handle);
 
-	lock = class_handle2object(handle->cookie, NULL);
+	lock = class_handle2object(handle->cookie, &lock_handle_ops);
 	if (!lock)
 		return NULL;
 
diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
index fc7985a2922c..b5fce96e43f7 100644
--- a/drivers/staging/lustre/lustre/obdclass/genops.c
+++ b/drivers/staging/lustre/lustre/obdclass/genops.c
@@ -674,6 +674,7 @@  int obd_init_caches(void)
 	return -ENOMEM;
 }
 
+static struct portals_handle_ops export_handle_ops;
 /* map connection to client */
 struct obd_export *class_conn2export(struct lustre_handle *conn)
 {
@@ -690,7 +691,7 @@  struct obd_export *class_conn2export(struct lustre_handle *conn)
 	}
 
 	CDEBUG(D_INFO, "looking for export cookie %#llx\n", conn->cookie);
-	export = class_handle2object(conn->cookie, NULL);
+	export = class_handle2object(conn->cookie, &export_handle_ops);
 	return export;
 }
 EXPORT_SYMBOL(class_conn2export);
diff --git a/drivers/staging/lustre/lustre/obdclass/lustre_handles.c b/drivers/staging/lustre/lustre/obdclass/lustre_handles.c
index 0674afb0059f..32b70d613f71 100644
--- a/drivers/staging/lustre/lustre/obdclass/lustre_handles.c
+++ b/drivers/staging/lustre/lustre/obdclass/lustre_handles.c
@@ -59,7 +59,7 @@  static struct handle_bucket {
  * global (per-node) hash-table.
  */
 void class_handle_hash(struct portals_handle *h,
-		       struct portals_handle_ops *ops)
+		       const struct portals_handle_ops *ops)
 {
 	struct handle_bucket *bucket;
 
@@ -132,7 +132,7 @@  void class_handle_unhash(struct portals_handle *h)
 }
 EXPORT_SYMBOL(class_handle_unhash);
 
-void *class_handle2object(u64 cookie, const void *owner)
+void *class_handle2object(u64 cookie, const struct portals_handle_ops *ops)
 {
 	struct handle_bucket *bucket;
 	struct portals_handle *h;
@@ -147,7 +147,7 @@  void *class_handle2object(u64 cookie, const void *owner)
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(h, &bucket->head, h_link) {
-		if (h->h_cookie != cookie || h->h_owner != owner)
+		if (h->h_cookie != cookie || h->h_ops != ops)
 			continue;
 
 		spin_lock(&h->h_lock);