| Message ID | 155053494646.24125.10155576042684274174.stgit@noble.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | More lustre patches from obdclass | expand |
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
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
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
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);
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(-)