Re: [RFC 2/4] bpf, security: Add Checmate

[zhuyj]

 Sure.
Why are preempt_disable and rcu_read_lock used here? is there a great
benefit of dong this?

On Fri, Aug 5, 2016 at 3:05 PM, Sargun Dhillon  wrote:
> On Thu, Aug 04, 2016 at 05:34:32PM +0800, zhuyj wrote:
>>  Sure.
>> Is it better to add
>> #ifndef CONFIG_PREEMPT_RCU ?
>>
>> On Thu, Aug 4, 2016 at 4:28 PM, Eric Dumazet  wrote:
>> > Please do not top post
>> >
>> > On Thu, 2016-08-04 at 16:08 +0800, zhuyj wrote:
>> >>  +void register_checmate_prog_ops(void);
>> >> maybe it is extern void register_checmate_prog_ops(void);?
>> >>
>> >> +   preempt_disable();
>> >> +   rcu_read_lock();
>> >> IMHO, it is not necessary to use the above 2 since rcu_read_lock will
>> >> call preempt_disable.
>> >
>> > You might double check if this claim is true if CONFIG_PREEMPT_RCU=y
>> >
>> >
>> >
> Thanks for your feedback zhuyj, Looking at kernel documentation itself, it 
> looks
> like this is the preferred mechanism[1]. Their example:
>
>  1 preempt_disable();
>  2 rcu_read_lock();
>  3 do_something();
>  4 rcu_read_unlock();
>  5 preempt_enable();
>
> But, I think you're right. Do you know if there's a great benefit of doing 
> this?
> Or does it make sense to implement a new macro, a la
> rcu_read_lock_and_preent_disable()?
>
> [1] 
> https://www.kernel.org/doc/Documentation/RCU/Design/Requirements/Requirements.html#Disabling
>  Preemption Does Not Block Grace Periods

Re: [RFC 2/4] bpf, security: Add Checmate

[zhuyj]

 Sure.
Why are preempt_disable and rcu_read_lock used here? is there a great
benefit of dong this?

On Fri, Aug 5, 2016 at 3:05 PM, Sargun Dhillon  wrote:
> On Thu, Aug 04, 2016 at 05:34:32PM +0800, zhuyj wrote:
>>  Sure.
>> Is it better to add
>> #ifndef CONFIG_PREEMPT_RCU ?
>>
>> On Thu, Aug 4, 2016 at 4:28 PM, Eric Dumazet  wrote:
>> > Please do not top post
>> >
>> > On Thu, 2016-08-04 at 16:08 +0800, zhuyj wrote:
>> >>  +void register_checmate_prog_ops(void);
>> >> maybe it is extern void register_checmate_prog_ops(void);?
>> >>
>> >> +   preempt_disable();
>> >> +   rcu_read_lock();
>> >> IMHO, it is not necessary to use the above 2 since rcu_read_lock will
>> >> call preempt_disable.
>> >
>> > You might double check if this claim is true if CONFIG_PREEMPT_RCU=y
>> >
>> >
>> >
> Thanks for your feedback zhuyj, Looking at kernel documentation itself, it 
> looks
> like this is the preferred mechanism[1]. Their example:
>
>  1 preempt_disable();
>  2 rcu_read_lock();
>  3 do_something();
>  4 rcu_read_unlock();
>  5 preempt_enable();
>
> But, I think you're right. Do you know if there's a great benefit of doing 
> this?
> Or does it make sense to implement a new macro, a la
> rcu_read_lock_and_preent_disable()?
>
> [1] 
> https://www.kernel.org/doc/Documentation/RCU/Design/Requirements/Requirements.html#Disabling
>  Preemption Does Not Block Grace Periods

Re: [RFC 2/4] bpf, security: Add Checmate

[Sargun Dhillon]

On Thu, Aug 04, 2016 at 05:34:32PM +0800, zhuyj wrote:
>  Sure.
> Is it better to add
> #ifndef CONFIG_PREEMPT_RCU ?
> 
> On Thu, Aug 4, 2016 at 4:28 PM, Eric Dumazet  wrote:
> > Please do not top post
> >
> > On Thu, 2016-08-04 at 16:08 +0800, zhuyj wrote:
> >>  +void register_checmate_prog_ops(void);
> >> maybe it is extern void register_checmate_prog_ops(void);?
> >>
> >> +   preempt_disable();
> >> +   rcu_read_lock();
> >> IMHO, it is not necessary to use the above 2 since rcu_read_lock will
> >> call preempt_disable.
> >
> > You might double check if this claim is true if CONFIG_PREEMPT_RCU=y
> >
> >
> >
Thanks for your feedback zhuyj, Looking at kernel documentation itself, it 
looks 
like this is the preferred mechanism[1]. Their example:

 1 preempt_disable();
 2 rcu_read_lock();
 3 do_something();
 4 rcu_read_unlock();
 5 preempt_enable();

But, I think you're right. Do you know if there's a great benefit of doing 
this? 
Or does it make sense to implement a new macro, a la 
rcu_read_lock_and_preent_disable()?

[1] 
https://www.kernel.org/doc/Documentation/RCU/Design/Requirements/Requirements.html#Disabling
 Preemption Does Not Block Grace Periods

Re: [RFC 2/4] bpf, security: Add Checmate

[Sargun Dhillon]

On Thu, Aug 04, 2016 at 05:34:32PM +0800, zhuyj wrote:
>  Sure.
> Is it better to add
> #ifndef CONFIG_PREEMPT_RCU ?
> 
> On Thu, Aug 4, 2016 at 4:28 PM, Eric Dumazet  wrote:
> > Please do not top post
> >
> > On Thu, 2016-08-04 at 16:08 +0800, zhuyj wrote:
> >>  +void register_checmate_prog_ops(void);
> >> maybe it is extern void register_checmate_prog_ops(void);?
> >>
> >> +   preempt_disable();
> >> +   rcu_read_lock();
> >> IMHO, it is not necessary to use the above 2 since rcu_read_lock will
> >> call preempt_disable.
> >
> > You might double check if this claim is true if CONFIG_PREEMPT_RCU=y
> >
> >
> >
Thanks for your feedback zhuyj, Looking at kernel documentation itself, it 
looks 
like this is the preferred mechanism[1]. Their example:

 1 preempt_disable();
 2 rcu_read_lock();
 3 do_something();
 4 rcu_read_unlock();
 5 preempt_enable();

But, I think you're right. Do you know if there's a great benefit of doing 
this? 
Or does it make sense to implement a new macro, a la 
rcu_read_lock_and_preent_disable()?

[1] 
https://www.kernel.org/doc/Documentation/RCU/Design/Requirements/Requirements.html#Disabling
 Preemption Does Not Block Grace Periods

Re: [RFC 2/4] bpf, security: Add Checmate

[zhuyj]

 Sure.
Is it better to add
#ifndef CONFIG_PREEMPT_RCU ?

On Thu, Aug 4, 2016 at 4:28 PM, Eric Dumazet  wrote:
> Please do not top post
>
> On Thu, 2016-08-04 at 16:08 +0800, zhuyj wrote:
>>  +void register_checmate_prog_ops(void);
>> maybe it is extern void register_checmate_prog_ops(void);?
>>
>> +   preempt_disable();
>> +   rcu_read_lock();
>> IMHO, it is not necessary to use the above 2 since rcu_read_lock will
>> call preempt_disable.
>
> You might double check if this claim is true if CONFIG_PREEMPT_RCU=y
>
>
>

Re: [RFC 2/4] bpf, security: Add Checmate

[zhuyj]

 Sure.
Is it better to add
#ifndef CONFIG_PREEMPT_RCU ?

On Thu, Aug 4, 2016 at 4:28 PM, Eric Dumazet  wrote:
> Please do not top post
>
> On Thu, 2016-08-04 at 16:08 +0800, zhuyj wrote:
>>  +void register_checmate_prog_ops(void);
>> maybe it is extern void register_checmate_prog_ops(void);?
>>
>> +   preempt_disable();
>> +   rcu_read_lock();
>> IMHO, it is not necessary to use the above 2 since rcu_read_lock will
>> call preempt_disable.
>
> You might double check if this claim is true if CONFIG_PREEMPT_RCU=y
>
>
>

Re: [RFC 2/4] bpf, security: Add Checmate

[Eric Dumazet]

Please do not top post

On Thu, 2016-08-04 at 16:08 +0800, zhuyj wrote:
>  +void register_checmate_prog_ops(void);
> maybe it is extern void register_checmate_prog_ops(void);?
> 
> +   preempt_disable();
> +   rcu_read_lock();
> IMHO, it is not necessary to use the above 2 since rcu_read_lock will
> call preempt_disable.

You might double check if this claim is true if CONFIG_PREEMPT_RCU=y

Re: [RFC 2/4] bpf, security: Add Checmate

[Eric Dumazet]

Please do not top post

On Thu, 2016-08-04 at 16:08 +0800, zhuyj wrote:
>  +void register_checmate_prog_ops(void);
> maybe it is extern void register_checmate_prog_ops(void);?
> 
> +   preempt_disable();
> +   rcu_read_lock();
> IMHO, it is not necessary to use the above 2 since rcu_read_lock will
> call preempt_disable.

You might double check if this claim is true if CONFIG_PREEMPT_RCU=y

Re: [RFC 2/4] bpf, security: Add Checmate

[zhuyj]

 +void register_checmate_prog_ops(void);
maybe it is extern void register_checmate_prog_ops(void);?

+   preempt_disable();
+   rcu_read_lock();
IMHO, it is not necessary to use the above 2 since rcu_read_lock will
call preempt_disable.

Zhu Yanjun

On Thu, Aug 4, 2016 at 3:11 PM, Sargun Dhillon  wrote:
> This adds the minor LSM Checmate. The purpose of Checmate is to act as an
> extensible LSM in which you can load security modules. The module has a
> simple API, as it's meant to have most of the logic in BPF hooks. It has
> three APIs that are accessible via prctl.
>
> As follows:
> * Install hook: This appends a new BPF program to a given hook. Hook
> programs themselves must be unique BPF programs.
> * Reset hook: This detaches all bpf programs asssociated with a hook.
> * Deny Reset: This locks a hook, preventing reset. In production
>   operation, it's expected that the user would lock
>   their hooks.
>
> Signed-off-by: Sargun Dhillon 
> ---
>  include/linux/checmate.h |  38 +
>  include/uapi/linux/Kbuild|   1 +
>  include/uapi/linux/bpf.h |   1 +
>  include/uapi/linux/checmate.h|  65 +
>  include/uapi/linux/prctl.h   |   3 +
>  security/Kconfig |   1 +
>  security/Makefile|   2 +
>  security/checmate/Kconfig|   6 +
>  security/checmate/Makefile   |   3 +
>  security/checmate/checmate_bpf.c |  67 +
>  security/checmate/checmate_lsm.c | 304 
> +++
>  11 files changed, 491 insertions(+)
>  create mode 100644 include/linux/checmate.h
>  create mode 100644 include/uapi/linux/checmate.h
>  create mode 100644 security/checmate/Kconfig
>  create mode 100644 security/checmate/Makefile
>  create mode 100644 security/checmate/checmate_bpf.c
>  create mode 100644 security/checmate/checmate_lsm.c
>
> diff --git a/include/linux/checmate.h b/include/linux/checmate.h
> new file mode 100644
> index 000..3e492b0
> --- /dev/null
> +++ b/include/linux/checmate.h
> @@ -0,0 +1,38 @@
> +#ifndef _LINUX_CHECMATE_H_
> +#define _LINUX_CHECMATE_H_ 1
> +#include 
> +#include 
> +
> +/* Miscellanious contexts */
> +struct checmate_file_open_ctx {
> +   struct file *file;
> +   const struct cred *cred;
> +};
> +
> +struct checmate_task_create_ctx {
> +   unsigned long clone_flags;
> +};
> +
> +struct checmate_task_free_ctx {
> +   struct task_struct *task;
> +};
> +
> +struct checmate_socket_connect_ctx {
> +   struct socket *sock;
> +   struct sockaddr *address;
> +   int addrlen;
> +};
> +
> +struct checmate_ctx {
> +   int hook;
> +   union {
> +   /* Miscellanious contexts */
> +   struct checmate_file_open_ctx   file_open_ctx;
> +   struct checmate_task_create_ctx 
> task_create_ctx;
> +   struct checmate_task_free_ctx   task_free_ctx;
> +   /* CONFIG_SECURITY_NET contexts */
> +   struct checmate_socket_connect_ctx  
> socket_connect_ctx;
> +   };
> +};
> +
> +#endif
> diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
> index ec10cfe..f8670a7 100644
> --- a/include/uapi/linux/Kbuild
> +++ b/include/uapi/linux/Kbuild
> @@ -82,6 +82,7 @@ header-y += cciss_defs.h
>  header-y += cciss_ioctl.h
>  header-y += cdrom.h
>  header-y += cgroupstats.h
> +header-y += checmate.h
>  header-y += chio.h
>  header-y += cm4000_cs.h
>  header-y += cn_proc.h
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index da218fe..6cafb58 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -95,6 +95,7 @@ enum bpf_prog_type {
> BPF_PROG_TYPE_SCHED_ACT,
> BPF_PROG_TYPE_TRACEPOINT,
> BPF_PROG_TYPE_XDP,
> +   BPF_PROG_TYPE_CHECMATE,
>  };
>
>  #define BPF_PSEUDO_MAP_FD  1
> diff --git a/include/uapi/linux/checmate.h b/include/uapi/linux/checmate.h
> new file mode 100644
> index 000..18af381
> --- /dev/null
> +++ b/include/uapi/linux/checmate.h
> @@ -0,0 +1,65 @@
> +#ifndef _UAPI__LINUX_CHECMATE_H__
> +#define _UAPI__LINUX_CHECMATE_H__
> +
> +#define CHECMATE_INSTALL_HOOK 1
> +#define CHECMATE_DENY_RESET 2
> +#define CHECMATE_RESET 3
> +
> +enum checmate_hook {
> +   CHECMATE_HOOK_UNSPEC,
> +   /* CONFIG_SECURITY_NET hooks */
> +   CHECMATE_HOOK_UNIX_STREAM_CONNECT,
> +   CHECMATE_HOOK_UNIX_MAY_SEND,
> +   CHECMATE_HOOK_SOCKET_CREATE,
> +   CHECMATE_HOOK_SOCKET_POST_CREATE,
> +   CHECMATE_HOOK_SOCKET_BIND,
> +   CHECMATE_HOOK_SOCKET_CONNECT,
> +   CHECMATE_HOOK_SOCKET_LISTEN,
> +   CHECMATE_HOOK_SOCKET_ACCEPT,
> +   CHECMATE_HOOK_SOCKET_SENDMSG,
> +   CHECMATE_HOOK_SOCKET_RECVMSG,
> +   CHECMATE_HOOK_SOCKET_GETSOCKNAME,
> +   CHECMATE_HOOK_SOCKET_GETPEERNAME,
> +   CHECMATE_HOOK_SOCKET_GETSOCKOPT,
> +   

Re: [RFC 2/4] bpf, security: Add Checmate

[zhuyj]

 +void register_checmate_prog_ops(void);
maybe it is extern void register_checmate_prog_ops(void);?

+   preempt_disable();
+   rcu_read_lock();
IMHO, it is not necessary to use the above 2 since rcu_read_lock will
call preempt_disable.

Zhu Yanjun

On Thu, Aug 4, 2016 at 3:11 PM, Sargun Dhillon  wrote:
> This adds the minor LSM Checmate. The purpose of Checmate is to act as an
> extensible LSM in which you can load security modules. The module has a
> simple API, as it's meant to have most of the logic in BPF hooks. It has
> three APIs that are accessible via prctl.
>
> As follows:
> * Install hook: This appends a new BPF program to a given hook. Hook
> programs themselves must be unique BPF programs.
> * Reset hook: This detaches all bpf programs asssociated with a hook.
> * Deny Reset: This locks a hook, preventing reset. In production
>   operation, it's expected that the user would lock
>   their hooks.
>
> Signed-off-by: Sargun Dhillon 
> ---
>  include/linux/checmate.h |  38 +
>  include/uapi/linux/Kbuild|   1 +
>  include/uapi/linux/bpf.h |   1 +
>  include/uapi/linux/checmate.h|  65 +
>  include/uapi/linux/prctl.h   |   3 +
>  security/Kconfig |   1 +
>  security/Makefile|   2 +
>  security/checmate/Kconfig|   6 +
>  security/checmate/Makefile   |   3 +
>  security/checmate/checmate_bpf.c |  67 +
>  security/checmate/checmate_lsm.c | 304 
> +++
>  11 files changed, 491 insertions(+)
>  create mode 100644 include/linux/checmate.h
>  create mode 100644 include/uapi/linux/checmate.h
>  create mode 100644 security/checmate/Kconfig
>  create mode 100644 security/checmate/Makefile
>  create mode 100644 security/checmate/checmate_bpf.c
>  create mode 100644 security/checmate/checmate_lsm.c
>
> diff --git a/include/linux/checmate.h b/include/linux/checmate.h
> new file mode 100644
> index 000..3e492b0
> --- /dev/null
> +++ b/include/linux/checmate.h
> @@ -0,0 +1,38 @@
> +#ifndef _LINUX_CHECMATE_H_
> +#define _LINUX_CHECMATE_H_ 1
> +#include 
> +#include 
> +
> +/* Miscellanious contexts */
> +struct checmate_file_open_ctx {
> +   struct file *file;
> +   const struct cred *cred;
> +};
> +
> +struct checmate_task_create_ctx {
> +   unsigned long clone_flags;
> +};
> +
> +struct checmate_task_free_ctx {
> +   struct task_struct *task;
> +};
> +
> +struct checmate_socket_connect_ctx {
> +   struct socket *sock;
> +   struct sockaddr *address;
> +   int addrlen;
> +};
> +
> +struct checmate_ctx {
> +   int hook;
> +   union {
> +   /* Miscellanious contexts */
> +   struct checmate_file_open_ctx   file_open_ctx;
> +   struct checmate_task_create_ctx 
> task_create_ctx;
> +   struct checmate_task_free_ctx   task_free_ctx;
> +   /* CONFIG_SECURITY_NET contexts */
> +   struct checmate_socket_connect_ctx  
> socket_connect_ctx;
> +   };
> +};
> +
> +#endif
> diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
> index ec10cfe..f8670a7 100644
> --- a/include/uapi/linux/Kbuild
> +++ b/include/uapi/linux/Kbuild
> @@ -82,6 +82,7 @@ header-y += cciss_defs.h
>  header-y += cciss_ioctl.h
>  header-y += cdrom.h
>  header-y += cgroupstats.h
> +header-y += checmate.h
>  header-y += chio.h
>  header-y += cm4000_cs.h
>  header-y += cn_proc.h
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index da218fe..6cafb58 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -95,6 +95,7 @@ enum bpf_prog_type {
> BPF_PROG_TYPE_SCHED_ACT,
> BPF_PROG_TYPE_TRACEPOINT,
> BPF_PROG_TYPE_XDP,
> +   BPF_PROG_TYPE_CHECMATE,
>  };
>
>  #define BPF_PSEUDO_MAP_FD  1
> diff --git a/include/uapi/linux/checmate.h b/include/uapi/linux/checmate.h
> new file mode 100644
> index 000..18af381
> --- /dev/null
> +++ b/include/uapi/linux/checmate.h
> @@ -0,0 +1,65 @@
> +#ifndef _UAPI__LINUX_CHECMATE_H__
> +#define _UAPI__LINUX_CHECMATE_H__
> +
> +#define CHECMATE_INSTALL_HOOK 1
> +#define CHECMATE_DENY_RESET 2
> +#define CHECMATE_RESET 3
> +
> +enum checmate_hook {
> +   CHECMATE_HOOK_UNSPEC,
> +   /* CONFIG_SECURITY_NET hooks */
> +   CHECMATE_HOOK_UNIX_STREAM_CONNECT,
> +   CHECMATE_HOOK_UNIX_MAY_SEND,
> +   CHECMATE_HOOK_SOCKET_CREATE,
> +   CHECMATE_HOOK_SOCKET_POST_CREATE,
> +   CHECMATE_HOOK_SOCKET_BIND,
> +   CHECMATE_HOOK_SOCKET_CONNECT,
> +   CHECMATE_HOOK_SOCKET_LISTEN,
> +   CHECMATE_HOOK_SOCKET_ACCEPT,
> +   CHECMATE_HOOK_SOCKET_SENDMSG,
> +   CHECMATE_HOOK_SOCKET_RECVMSG,
> +   CHECMATE_HOOK_SOCKET_GETSOCKNAME,
> +   CHECMATE_HOOK_SOCKET_GETPEERNAME,
> +   CHECMATE_HOOK_SOCKET_GETSOCKOPT,
> +   CHECMATE_HOOK_SOCKET_SETSOCKOPT,
> +