Re: [PATCH 1/4] inotify: Add infrastructure to account inotify limits per-namespace

2016-06-06 Thread Nikolay Borisov 


On 06/06/2016 11:05 AM, Cyrill Gorcunov wrote:
> On Wed, Jun 01, 2016 at 10:52:57AM +0300, Nikolay Borisov wrote:
>> This patch adds the necessary members to user_struct. The idea behind
>> the solution is really simple - user the userns pointers as keys into
>> a hash table which holds the inotify instances/watches counts. This
>> allows to account the limits per userns rather than per real user,
>> which makes certain scenarios such as a single mapped user in a
>> container deplete the inotify resources for all other users, which
>> map to the exact same real user.
>>
>> Signed-off-by: Nikolay Borisov 
> ...
>> +static inline unsigned long inotify_dec_return_dev(struct user_struct *user,
>> +   void *key)
>> +{
>> +struct inotify_state *state;
>> +unsigned long ret;
>> +
>> +spin_lock(>inotify_lock);
>> +state = __find_inotify_state(user, key);
>> +ret = --state->inotify_devs;
>> +spin_unlock(>inotify_lock);
>> +
>> +return ret;
>> +}
> 
> Hi Nikolay! Could you please explain why this new function is not used 
> anywhere
> in other patches or I miss something obvious?

Hi Cyrill,

It seems this is a left-over from an earlier, internal version of this
patchset. You can disregard it. Also, given the direction that the
discussion with Eric took I think I will be redesigning the solution
entirely. Thanks for taking the time to read the code!


Nikolay

Re: [PATCH 1/4] inotify: Add infrastructure to account inotify limits per-namespace

2016-06-06 Thread Cyrill Gorcunov 
On Wed, Jun 01, 2016 at 10:52:57AM +0300, Nikolay Borisov wrote:
> This patch adds the necessary members to user_struct. The idea behind
> the solution is really simple - user the userns pointers as keys into
> a hash table which holds the inotify instances/watches counts. This
> allows to account the limits per userns rather than per real user,
> which makes certain scenarios such as a single mapped user in a
> container deplete the inotify resources for all other users, which
> map to the exact same real user.
> 
> Signed-off-by: Nikolay Borisov 
...
> +static inline unsigned long inotify_dec_return_dev(struct user_struct *user,
> +void *key)
> +{
> + struct inotify_state *state;
> + unsigned long ret;
> +
> + spin_lock(>inotify_lock);
> + state = __find_inotify_state(user, key);
> + ret = --state->inotify_devs;
> + spin_unlock(>inotify_lock);
> +
> + return ret;
> +}

Hi Nikolay! Could you please explain why this new function is not used anywhere
in other patches or I miss something obvious?

[PATCH 1/4] inotify: Add infrastructure to account inotify limits per-namespace

2016-06-01 Thread Nikolay Borisov 
This patch adds the necessary members to user_struct. The idea behind
the solution is really simple - user the userns pointers as keys into
a hash table which holds the inotify instances/watches counts. This
allows to account the limits per userns rather than per real user,
which makes certain scenarios such as a single mapped user in a
container deplete the inotify resources for all other users, which
map to the exact same real user.

Signed-off-by: Nikolay Borisov 
---
 fs/notify/inotify/inotify.h  | 68 
 fs/notify/inotify/inotify_user.c | 36 +
 include/linux/fsnotify_backend.h |  1 +
 include/linux/sched.h|  3 ++
 kernel/user.c| 13 
 5 files changed, 121 insertions(+)

diff --git a/fs/notify/inotify/inotify.h b/fs/notify/inotify/inotify.h
index ed855ef6f077..e069e1e4262a 100644
--- a/fs/notify/inotify/inotify.h
+++ b/fs/notify/inotify/inotify.h
@@ -1,6 +1,7 @@
 #include 
 #include 
 #include  /* struct kmem_cache */
+#include 
 
 struct inotify_event_info {
struct fsnotify_event fse;
@@ -15,6 +16,13 @@ struct inotify_inode_mark {
int wd;
 };
 
+struct inotify_state {
+   struct hlist_node node;
+   void *key; /* user_namespace ptr */
+   u32 inotify_watches; /* How many inotify watches does this user have? */
+   u32 inotify_devs;  /* How many inotify devs does this user have opened? 
*/
+};
+
 static inline struct inotify_event_info *INOTIFY_E(struct fsnotify_event *fse)
 {
return container_of(fse, struct inotify_event_info, fse);
@@ -30,3 +38,63 @@ extern int inotify_handle_event(struct fsnotify_group *group,
const unsigned char *file_name, u32 cookie);
 
 extern const struct fsnotify_ops inotify_fsnotify_ops;
+
+/* Helpers for manipulating various inotify state, stored in user_struct */
+static inline struct inotify_state *__find_inotify_state(struct user_struct 
*user,
+ void *key)
+{
+   struct inotify_state *state;
+
+   hash_for_each_possible(user->inotify_tbl, state, node, (unsigned 
long)key)
+   if (state->key == key)
+   return state;
+
+   return NULL;
+}
+
+static inline void inotify_inc_watches(struct user_struct *user, void *key)
+{
+   struct inotify_state *state;
+
+   spin_lock(>inotify_lock);
+   state = __find_inotify_state(user, key);
+   state->inotify_watches++;
+   spin_unlock(>inotify_lock);
+}
+
+
+static inline void inotify_dec_watches(struct user_struct *user, void *key)
+{
+   struct inotify_state *state;
+
+   spin_lock(>inotify_lock);
+   state = __find_inotify_state(user, key);
+   state->inotify_watches--;
+   spin_unlock(>inotify_lock);
+}
+
+static inline int inotify_read_watches(struct user_struct *user, void *key)
+{
+   struct inotify_state *state;
+   int ret;
+
+   spin_lock(>inotify_lock);
+   state = __find_inotify_state(user, key);
+   ret = state->inotify_watches;
+   spin_unlock(>inotify_lock);
+   return ret;
+}
+
+static inline unsigned long inotify_dec_return_dev(struct user_struct *user,
+  void *key)
+{
+   struct inotify_state *state;
+   unsigned long ret;
+
+   spin_lock(>inotify_lock);
+   state = __find_inotify_state(user, key);
+   ret = --state->inotify_devs;
+   spin_unlock(>inotify_lock);
+
+   return ret;
+}
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index b8d08d0d0a4d..ae7ec2414252 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -86,6 +86,42 @@ struct ctl_table inotify_table[] = {
 };
 #endif /* CONFIG_SYSCTL */
 
+
+static int inotify_init_state(struct user_struct *user,
+ void *key)
+{
+   struct inotify_state *state;
+   int ret = 0;
+
+   spin_lock(>inotify_lock);
+   state =  __find_inotify_count(user, key);
+
+   if (!state) {
+   spin_unlock(>inotify_lock);
+   state = kzalloc(sizeof(struct inotify_state), GFP_KERNEL);
+   if (!state)
+   return -ENOMEM;
+
+   state->key = current_user_ns();
+   state->inotify_watches = 0;
+   state->inotify_devs = 1;
+
+   spin_lock(>inotify_lock);
+   hash_add(user->inotify_tbl, >node, (unsigned long)key);
+
+   goto out;
+   } else {
+
+   if (++state->inotify_devs > inotify_max_user_instances) {
+   ret = -EMFILE;
+   goto out;
+   }
+   }
+out:
+   spin_unlock(>inotify_lock);
+   return ret;
+}
+
 static inline __u32 inotify_arg_to_mask(u32 arg)
 {
__u32 mask;
diff --git a/include/linux/fsnotify_backend.h