[bpf-next,2/2] tools: bpftool: add "bpftool map freeze" subcommand
diff mbox series

Message ID 20190821085219.30387-3-quentin.monnet@netronome.com
State Accepted
Delegated to: BPF Maintainers
Headers show
Series
  • tools: bpftool: work with frozen maps
Related show

Commit Message

Quentin Monnet Aug. 21, 2019, 8:52 a.m. UTC
Add a new subcommand to freeze maps from user space.

Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 .../bpf/bpftool/Documentation/bpftool-map.rst |  9 +++++
 tools/bpf/bpftool/bash-completion/bpftool     |  4 +--
 tools/bpf/bpftool/map.c                       | 34 ++++++++++++++++++-
 3 files changed, 44 insertions(+), 3 deletions(-)

Comments

Daniel Borkmann Aug. 21, 2019, 11:40 a.m. UTC | #1
On 8/21/19 10:52 AM, Quentin Monnet wrote:
> Add a new subcommand to freeze maps from user space.
> 
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
>   .../bpf/bpftool/Documentation/bpftool-map.rst |  9 +++++
>   tools/bpf/bpftool/bash-completion/bpftool     |  4 +--
>   tools/bpf/bpftool/map.c                       | 34 ++++++++++++++++++-
>   3 files changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst
> index 61d1d270eb5e..1c0f7146aab0 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
> @@ -36,6 +36,7 @@ MAP COMMANDS
>   |	**bpftool** **map pop**        *MAP*
>   |	**bpftool** **map enqueue**    *MAP* **value** *VALUE*
>   |	**bpftool** **map dequeue**    *MAP*
> +|	**bpftool** **map freeze**     *MAP*
>   |	**bpftool** **map help**
>   |
>   |	*MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
> @@ -127,6 +128,14 @@ DESCRIPTION
>   	**bpftool map dequeue**  *MAP*
>   		  Dequeue and print **value** from the queue.
>   
> +	**bpftool map freeze**  *MAP*
> +		  Freeze the map as read-only from user space. Entries from a
> +		  frozen map can not longer be updated or deleted with the
> +		  **bpf\ ()** system call. This operation is not reversible,
> +		  and the map remains immutable from user space until its
> +		  destruction. However, read and write permissions for BPF
> +		  programs to the map remain unchanged.

That is not correct, programs that are loaded into the system /after/ the map
has been frozen cannot modify values either, thus read-only from both sides.

Thanks,
Daniel
Quentin Monnet Aug. 21, 2019, 12:58 p.m. UTC | #2
2019-08-21 13:40 UTC+0200 ~ Daniel Borkmann <daniel@iogearbox.net>
> On 8/21/19 10:52 AM, Quentin Monnet wrote:
>> Add a new subcommand to freeze maps from user space.
>>
>> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
>> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>> ---
>>   .../bpf/bpftool/Documentation/bpftool-map.rst |  9 +++++
>>   tools/bpf/bpftool/bash-completion/bpftool     |  4 +--
>>   tools/bpf/bpftool/map.c                       | 34 ++++++++++++++++++-
>>   3 files changed, 44 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst
>> b/tools/bpf/bpftool/Documentation/bpftool-map.rst
>> index 61d1d270eb5e..1c0f7146aab0 100644
>> --- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
>> +++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
>> @@ -36,6 +36,7 @@ MAP COMMANDS
>>   |    **bpftool** **map pop**        *MAP*
>>   |    **bpftool** **map enqueue**    *MAP* **value** *VALUE*
>>   |    **bpftool** **map dequeue**    *MAP*
>> +|    **bpftool** **map freeze**     *MAP*
>>   |    **bpftool** **map help**
>>   |
>>   |    *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
>> @@ -127,6 +128,14 @@ DESCRIPTION
>>       **bpftool map dequeue**  *MAP*
>>             Dequeue and print **value** from the queue.
>>   +    **bpftool map freeze**  *MAP*
>> +          Freeze the map as read-only from user space. Entries from a
>> +          frozen map can not longer be updated or deleted with the
>> +          **bpf\ ()** system call. This operation is not reversible,
>> +          and the map remains immutable from user space until its
>> +          destruction. However, read and write permissions for BPF
>> +          programs to the map remain unchanged.
> 
> That is not correct, programs that are loaded into the system /after/
> the map
> has been frozen cannot modify values either, thus read-only from both
> sides.
> 
> Thanks,
> Daniel

Hi Daniel,

Are you entirely sure about it? I could not find the relevant
restriction in the code, the checks seem to be on map flags
(BPF_F_RDONLY) which do not seem to be modified by the "frozen" status
in map_freeze()? And tests I ran on my side seem to indicate the map can
still be updated by new programs. Did I miss something?

Tested on 5.3.0-rc1:

1. Create hash map
2. Load BPF program foo, using map
3. Test-run program foo - map is updated
4. Freeze map - update effectively becomes impossible from user space
5. Load BPF program bar, using same map
6. Test-run program bar - map is still updated

Best regards,
Quentin
Daniel Borkmann Aug. 21, 2019, 1:08 p.m. UTC | #3
On 8/21/19 2:58 PM, Quentin Monnet wrote:
> 2019-08-21 13:40 UTC+0200 ~ Daniel Borkmann <daniel@iogearbox.net>
>> On 8/21/19 10:52 AM, Quentin Monnet wrote:
>>> Add a new subcommand to freeze maps from user space.
>>>
>>> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
>>> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>>> ---
>>>    .../bpf/bpftool/Documentation/bpftool-map.rst |  9 +++++
>>>    tools/bpf/bpftool/bash-completion/bpftool     |  4 +--
>>>    tools/bpf/bpftool/map.c                       | 34 ++++++++++++++++++-
>>>    3 files changed, 44 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst
>>> b/tools/bpf/bpftool/Documentation/bpftool-map.rst
>>> index 61d1d270eb5e..1c0f7146aab0 100644
>>> --- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
>>> +++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
>>> @@ -36,6 +36,7 @@ MAP COMMANDS
>>>    |    **bpftool** **map pop**        *MAP*
>>>    |    **bpftool** **map enqueue**    *MAP* **value** *VALUE*
>>>    |    **bpftool** **map dequeue**    *MAP*
>>> +|    **bpftool** **map freeze**     *MAP*
>>>    |    **bpftool** **map help**
>>>    |
>>>    |    *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
>>> @@ -127,6 +128,14 @@ DESCRIPTION
>>>        **bpftool map dequeue**  *MAP*
>>>              Dequeue and print **value** from the queue.
>>>    +    **bpftool map freeze**  *MAP*
>>> +          Freeze the map as read-only from user space. Entries from a
>>> +          frozen map can not longer be updated or deleted with the
>>> +          **bpf\ ()** system call. This operation is not reversible,
>>> +          and the map remains immutable from user space until its
>>> +          destruction. However, read and write permissions for BPF
>>> +          programs to the map remain unchanged.
>>
>> That is not correct, programs that are loaded into the system /after/
>> the map
>> has been frozen cannot modify values either, thus read-only from both
>> sides.
> 
> Are you entirely sure about it? I could not find the relevant
> restriction in the code, the checks seem to be on map flags
> (BPF_F_RDONLY) which do not seem to be modified by the "frozen" status
> in map_freeze()? And tests I ran on my side seem to indicate the map can
> still be updated by new programs. Did I miss something?
> 
> Tested on 5.3.0-rc1:
> 
> 1. Create hash map
> 2. Load BPF program foo, using map
> 3. Test-run program foo - map is updated
> 4. Freeze map - update effectively becomes impossible from user space
> 5. Load BPF program bar, using same map
> 6. Test-run program bar - map is still updated

Looks like I need some more coffee. ;-) Indeed, the program side was via
BPF_F_RDONLY_PROG flag.

Patch
diff mbox series

diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst
index 61d1d270eb5e..1c0f7146aab0 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
@@ -36,6 +36,7 @@  MAP COMMANDS
 |	**bpftool** **map pop**        *MAP*
 |	**bpftool** **map enqueue**    *MAP* **value** *VALUE*
 |	**bpftool** **map dequeue**    *MAP*
+|	**bpftool** **map freeze**     *MAP*
 |	**bpftool** **map help**
 |
 |	*MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
@@ -127,6 +128,14 @@  DESCRIPTION
 	**bpftool map dequeue**  *MAP*
 		  Dequeue and print **value** from the queue.
 
+	**bpftool map freeze**  *MAP*
+		  Freeze the map as read-only from user space. Entries from a
+		  frozen map can not longer be updated or deleted with the
+		  **bpf\ ()** system call. This operation is not reversible,
+		  and the map remains immutable from user space until its
+		  destruction. However, read and write permissions for BPF
+		  programs to the map remain unchanged.
+
 	**bpftool map help**
 		  Print short help message.
 
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index 2ffd351f9dbf..70493a6da206 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -449,7 +449,7 @@  _bpftool()
         map)
             local MAP_TYPE='id pinned'
             case $command in
-                show|list|dump|peek|pop|dequeue)
+                show|list|dump|peek|pop|dequeue|freeze)
                     case $prev in
                         $command)
                             COMPREPLY=( $( compgen -W "$MAP_TYPE" -- "$cur" ) )
@@ -638,7 +638,7 @@  _bpftool()
                     [[ $prev == $object ]] && \
                         COMPREPLY=( $( compgen -W 'delete dump getnext help \
                             lookup pin event_pipe show list update create \
-                            peek push enqueue pop dequeue' -- \
+                            peek push enqueue pop dequeue freeze' -- \
                             "$cur" ) )
                     ;;
             esac
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index af2e9eb9747b..de61d73b9030 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -1262,6 +1262,35 @@  static int do_pop_dequeue(int argc, char **argv)
 	return err;
 }
 
+static int do_freeze(int argc, char **argv)
+{
+	int err, fd;
+
+	if (!REQ_ARGS(2))
+		return -1;
+
+	fd = map_parse_fd(&argc, &argv);
+	if (fd < 0)
+		return -1;
+
+	if (argc) {
+		close(fd);
+		return BAD_ARG();
+	}
+
+	err = bpf_map_freeze(fd);
+	close(fd);
+	if (err) {
+		p_err("failed to freeze map: %s", strerror(errno));
+		return err;
+	}
+
+	if (json_output)
+		jsonw_null(json_wtr);
+
+	return 0;
+}
+
 static int do_help(int argc, char **argv)
 {
 	if (json_output) {
@@ -1286,6 +1315,7 @@  static int do_help(int argc, char **argv)
 		"       %s %s pop        MAP\n"
 		"       %s %s enqueue    MAP value VALUE\n"
 		"       %s %s dequeue    MAP\n"
+		"       %s %s freeze     MAP\n"
 		"       %s %s help\n"
 		"\n"
 		"       " HELP_SPEC_MAP "\n"
@@ -1304,7 +1334,8 @@  static int do_help(int argc, char **argv)
 		bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
 		bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
 		bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
-		bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2]);
+		bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
+		bin_name, argv[-2]);
 
 	return 0;
 }
@@ -1326,6 +1357,7 @@  static const struct cmd cmds[] = {
 	{ "enqueue",	do_update },
 	{ "pop",	do_pop_dequeue },
 	{ "dequeue",	do_pop_dequeue },
+	{ "freeze",	do_freeze },
 	{ 0 }
 };