| Message ID | 20220208051635.2160304-15-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Fix accessing syscall arguments | expand |
On Tue, Feb 08, 2022 at 06:16:35AM +0100, Ilya Leoshkevich wrote: > orig_x0's location is used by libbpf tracing macros, therefore it > should not be moved. > > Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- > arch/arm64/include/asm/ptrace.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h > index 41b332c054ab..7e34c3737839 100644 > --- a/arch/arm64/include/asm/ptrace.h > +++ b/arch/arm64/include/asm/ptrace.h > @@ -185,6 +185,10 @@ struct pt_regs { > u64 pstate; > }; > }; > + /* > + * orig_x0 is not exposed via struct user_pt_regs, but its location is > + * assumed by libbpf's tracing macros, so it should not be moved. > + */ In other words this comment is saying that the layout is ABI. That's not the case. orig_x0 here and equivalent on s390 can be moved. It will break bpf progs written without CO-RE and that is expected. Non CO-RE programs often do all kinds of bpf_probe_read_kernel and will be breaking when kernel layout is changing. I suggest to drop this patch and patch 12.
On Tue, 2022-02-08 at 11:25 -0800, Alexei Starovoitov wrote: > On Tue, Feb 08, 2022 at 06:16:35AM +0100, Ilya Leoshkevich wrote: > > orig_x0's location is used by libbpf tracing macros, therefore it > > should not be moved. > > > > Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com> > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > --- > > arch/arm64/include/asm/ptrace.h | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/arch/arm64/include/asm/ptrace.h > > b/arch/arm64/include/asm/ptrace.h > > index 41b332c054ab..7e34c3737839 100644 > > --- a/arch/arm64/include/asm/ptrace.h > > +++ b/arch/arm64/include/asm/ptrace.h > > @@ -185,6 +185,10 @@ struct pt_regs { > > u64 pstate; > > }; > > }; > > + /* > > + * orig_x0 is not exposed via struct user_pt_regs, but its > > location is > > + * assumed by libbpf's tracing macros, so it should not be > > moved. > > + */ > > In other words this comment is saying that the layout is ABI. > That's not the case. orig_x0 here and equivalent on s390 can be > moved. > It will break bpf progs written without CO-RE and that is expected. > Non CO-RE programs often do all kinds of bpf_probe_read_kernel and > will be breaking when kernel layout is changing. > I suggest to drop this patch and patch 12. Yeah, that was the intention here: to promote orig_x0 to ABI using a comment, since doing this by extending user_pt_regs turned out to be infeasible. I'm actually ok with not doing this, since programs compiled with kernel headers and using CO-RE macros will be fine. As you say, we don't care about programs that don't use CO-RE too much here - if they break after an incompatible kernel change, fine. The question now is - how much do we care about programs that are compiled with userspace headers? Andrii suggested to use offsetofend to make syscall macros work there, however, this now requires this ABI promotion.
On Tue, Feb 8, 2022 at 11:46 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > On Tue, 2022-02-08 at 11:25 -0800, Alexei Starovoitov wrote: > > On Tue, Feb 08, 2022 at 06:16:35AM +0100, Ilya Leoshkevich wrote: > > > orig_x0's location is used by libbpf tracing macros, therefore it > > > should not be moved. > > > > > > Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com> > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > > --- > > > arch/arm64/include/asm/ptrace.h | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/arch/arm64/include/asm/ptrace.h > > > b/arch/arm64/include/asm/ptrace.h > > > index 41b332c054ab..7e34c3737839 100644 > > > --- a/arch/arm64/include/asm/ptrace.h > > > +++ b/arch/arm64/include/asm/ptrace.h > > > @@ -185,6 +185,10 @@ struct pt_regs { > > > u64 pstate; > > > }; > > > }; > > > + /* > > > + * orig_x0 is not exposed via struct user_pt_regs, but its > > > location is > > > + * assumed by libbpf's tracing macros, so it should not be > > > moved. > > > + */ > > > > In other words this comment is saying that the layout is ABI. > > That's not the case. orig_x0 here and equivalent on s390 can be > > moved. > > It will break bpf progs written without CO-RE and that is expected. > > Non CO-RE programs often do all kinds of bpf_probe_read_kernel and > > will be breaking when kernel layout is changing. > > I suggest to drop this patch and patch 12. > > Yeah, that was the intention here: to promote orig_x0 to ABI using a > comment, since doing this by extending user_pt_regs turned out to be > infeasible. I'm actually ok with not doing this, since programs > compiled with kernel headers and using CO-RE macros will be fine. The comment like this doesn't convert kernel internal struct into ABI. The comment is just wrong. BPF progs access many kernel data structs. s390's and arm64's struct pr_regs is not special in that sense. It's an internal struct. > As you say, we don't care about programs that don't use CO-RE too much > here - if they break after an incompatible kernel change, fine. Before CO-RE was introduced bpf progs included kernel headers and were breaking when kernel changes. Nothing new here. See the history of bcc tools. Some of them are littered with ifdef VERSION ==. > The question now is - how much do we care about programs that are > compiled with userspace headers? Andrii suggested to use offsetofend to > make syscall macros work there, however, this now requires this ABI > promotion. Today s390 and arm64 have user_pt_regs as a first field in pt_regs. That is kernel internal behavior and that part can change if arch maintainers have a need for that. bpf progs without CO-RE would have to be adjusted when kernel changes. Even with CO-RE it's ok to rename pt_regs->orig_gpr2 on s390. The progs with CO-RE will break too. The authors of tracing bpf progs have to expect that sooner or later their progs will break and they would have to adjust them.
On Tue, 2022-02-08 at 13:11 -0800, Alexei Starovoitov wrote: > On Tue, Feb 8, 2022 at 11:46 AM Ilya Leoshkevich <iii@linux.ibm.com> > wrote: > > > > On Tue, 2022-02-08 at 11:25 -0800, Alexei Starovoitov wrote: > > > On Tue, Feb 08, 2022 at 06:16:35AM +0100, Ilya Leoshkevich wrote: > > > > orig_x0's location is used by libbpf tracing macros, therefore > > > > it > > > > should not be moved. > > > > > > > > Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com> > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > > > --- > > > > arch/arm64/include/asm/ptrace.h | 4 ++++ > > > > 1 file changed, 4 insertions(+) > > > > > > > > diff --git a/arch/arm64/include/asm/ptrace.h > > > > b/arch/arm64/include/asm/ptrace.h > > > > index 41b332c054ab..7e34c3737839 100644 > > > > --- a/arch/arm64/include/asm/ptrace.h > > > > +++ b/arch/arm64/include/asm/ptrace.h > > > > @@ -185,6 +185,10 @@ struct pt_regs { > > > > u64 pstate; > > > > }; > > > > }; > > > > + /* > > > > + * orig_x0 is not exposed via struct user_pt_regs, but > > > > its > > > > location is > > > > + * assumed by libbpf's tracing macros, so it should not > > > > be > > > > moved. > > > > + */ > > > > > > In other words this comment is saying that the layout is ABI. > > > That's not the case. orig_x0 here and equivalent on s390 can be > > > moved. > > > It will break bpf progs written without CO-RE and that is > > > expected. > > > Non CO-RE programs often do all kinds of bpf_probe_read_kernel > > > and > > > will be breaking when kernel layout is changing. > > > I suggest to drop this patch and patch 12. > > > > Yeah, that was the intention here: to promote orig_x0 to ABI using > > a > > comment, since doing this by extending user_pt_regs turned out to > > be > > infeasible. I'm actually ok with not doing this, since programs > > compiled with kernel headers and using CO-RE macros will be fine. > > The comment like this doesn't convert kernel internal struct into > ABI. > The comment is just wrong. BPF progs access many kernel data structs. > s390's and arm64's struct pr_regs is not special in that sense. > It's an internal struct. > > > As you say, we don't care about programs that don't use CO-RE too > > much > > here - if they break after an incompatible kernel change, fine. > > Before CO-RE was introduced bpf progs included kernel headers > and were breaking when kernel changes. Nothing new here. > See the history of bcc tools. Some of them are littered > with ifdef VERSION ==. > > > The question now is - how much do we care about programs that are > > > compiled with userspace headers? Andrii suggested to use > > offsetofend to > > make syscall macros work there, however, this now requires this ABI > > promotion. > > Today s390 and arm64 have user_pt_regs as a first field in pt_regs. > That is kernel internal behavior and that part can change if arch > maintainers have a need for that. > bpf progs without CO-RE would have to be adjusted when kernel > changes. > Even with CO-RE it's ok to rename pt_regs->orig_gpr2 on s390. > The progs with CO-RE will break too. The authors of tracing bpf progs > have to expect that sooner or later their progs will break and they > would have to adjust them. When it comes to authors of tracing bpf progs, I agree that eventually they will have to adjust their code, CO-RE or not. However, in patch 13 I introduce the following libbpf macro: #if defined(__KERNEL__) || defined(__VMLINUX_H__) ... #else #define PT_REGS_PARM1_SYSCALL(x) \ (*(unsigned long *)(((char *)(x) + \ offsetofend(struct user_pt_regs, pstate)))) If we merge this series without freezing orig_x0's offset, in case of an incompatible kernel change the users of PT_REGS_PARMn_SYSCALL and BPF_KPROBE_SYSCALL, who build against userspace headers, will not simply have to update their code - they will have to upgrade libbpf. It's also not clear how PT_REGS_PARM1_SYSCALL in the upgraded libbpf will even look like, given that it would need to work on both old and new kernels. I've also briefly looked into MIPS' ptrace.h, and it looks as if their user_pt_regs has no relationship to kernel pt_regs. IIUC this means that it's not possible to correctly implement PT_REGS_PARMn_SYSCALL using MIPS userspace headers. So I wonder whether we should allow using PT_REGS_PARMn_SYSCALL and BPF_KPROBE_SYSCALL with userspace headers at all? Would it make sense to simply fail the compilation if PT_REGS_PARMn_SYSCALL is used without including kernel headers?
On Tue, Feb 8, 2022 at 1:46 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > On Tue, 2022-02-08 at 13:11 -0800, Alexei Starovoitov wrote: > > On Tue, Feb 8, 2022 at 11:46 AM Ilya Leoshkevich <iii@linux.ibm.com> > > wrote: > > > > > > On Tue, 2022-02-08 at 11:25 -0800, Alexei Starovoitov wrote: > > > > On Tue, Feb 08, 2022 at 06:16:35AM +0100, Ilya Leoshkevich wrote: > > > > > orig_x0's location is used by libbpf tracing macros, therefore > > > > > it > > > > > should not be moved. > > > > > > > > > > Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com> > > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > > > > --- > > > > > arch/arm64/include/asm/ptrace.h | 4 ++++ > > > > > 1 file changed, 4 insertions(+) > > > > > > > > > > diff --git a/arch/arm64/include/asm/ptrace.h > > > > > b/arch/arm64/include/asm/ptrace.h > > > > > index 41b332c054ab..7e34c3737839 100644 > > > > > --- a/arch/arm64/include/asm/ptrace.h > > > > > +++ b/arch/arm64/include/asm/ptrace.h > > > > > @@ -185,6 +185,10 @@ struct pt_regs { > > > > > u64 pstate; > > > > > }; > > > > > }; > > > > > + /* > > > > > + * orig_x0 is not exposed via struct user_pt_regs, but > > > > > its > > > > > location is > > > > > + * assumed by libbpf's tracing macros, so it should not > > > > > be > > > > > moved. > > > > > + */ > > > > > > > > In other words this comment is saying that the layout is ABI. > > > > That's not the case. orig_x0 here and equivalent on s390 can be > > > > moved. > > > > It will break bpf progs written without CO-RE and that is > > > > expected. > > > > Non CO-RE programs often do all kinds of bpf_probe_read_kernel > > > > and > > > > will be breaking when kernel layout is changing. > > > > I suggest to drop this patch and patch 12. > > > > > > Yeah, that was the intention here: to promote orig_x0 to ABI using > > > a > > > comment, since doing this by extending user_pt_regs turned out to > > > be > > > infeasible. I'm actually ok with not doing this, since programs > > > compiled with kernel headers and using CO-RE macros will be fine. > > > > The comment like this doesn't convert kernel internal struct into > > ABI. > > The comment is just wrong. BPF progs access many kernel data structs. > > s390's and arm64's struct pr_regs is not special in that sense. > > It's an internal struct. > > > > > As you say, we don't care about programs that don't use CO-RE too > > > much > > > here - if they break after an incompatible kernel change, fine. > > > > Before CO-RE was introduced bpf progs included kernel headers > > and were breaking when kernel changes. Nothing new here. > > See the history of bcc tools. Some of them are littered > > with ifdef VERSION ==. > > > > > The question now is - how much do we care about programs that are > > > > > compiled with userspace headers? Andrii suggested to use > > > offsetofend to > > > make syscall macros work there, however, this now requires this ABI > > > promotion. > > > > Today s390 and arm64 have user_pt_regs as a first field in pt_regs. > > That is kernel internal behavior and that part can change if arch > > maintainers have a need for that. > > bpf progs without CO-RE would have to be adjusted when kernel > > changes. > > Even with CO-RE it's ok to rename pt_regs->orig_gpr2 on s390. > > The progs with CO-RE will break too. The authors of tracing bpf progs > > have to expect that sooner or later their progs will break and they > > would have to adjust them. > > When it comes to authors of tracing bpf progs, I agree that eventually > they will have to adjust their code, CO-RE or not. However, in patch 13 > I introduce the following libbpf macro: > > #if defined(__KERNEL__) || defined(__VMLINUX_H__) > ... > #else > #define PT_REGS_PARM1_SYSCALL(x) \ > (*(unsigned long *)(((char *)(x) + \ > offsetofend(struct user_pt_regs, > pstate)))) > > If we merge this series without freezing orig_x0's offset, in case of > an incompatible kernel change the users of PT_REGS_PARMn_SYSCALL and > BPF_KPROBE_SYSCALL, who build against userspace headers, will not > simply have to update their code - they will have to upgrade libbpf. > It's also not clear how PT_REGS_PARM1_SYSCALL in the upgraded libbpf > will even look like, given that it would need to work on both old and > new kernels. > > I've also briefly looked into MIPS' ptrace.h, and it looks as if their > user_pt_regs has no relationship to kernel pt_regs. IIUC this means > that it's not possible to correctly implement PT_REGS_PARMn_SYSCALL > using MIPS userspace headers. > > So I wonder whether we should allow using PT_REGS_PARMn_SYSCALL and > BPF_KPROBE_SYSCALL with userspace headers at all? Would it make sense > to simply fail the compilation if PT_REGS_PARMn_SYSCALL is used without > including kernel headers? Ok, my bad for suggesting those comments, I didn't realize the consequences of making anything into a stable ABI. Let's not add any comments, we don't need that. I think we should just come to terms that for some architectures this syscall argument access won't work at least for some architectures without CO-RE. For uniformity let's still have those PT_REGS_PARM1_SYSCALL macro defined, but we should use __bpf_unreachable or something like that to make sure it won't compile if someone tries to use it. But it's an entirely different story for CORE variants and there (as I explained on one of the previous patches) we can fabricate our own definitions of pt_regs (architecture specific, using CO-RE struct flavors) without any unnecessary assumptions about which include headers the user is going to use. Hengqi's BPF_KPROBE_SYSCALL() macro is always going to use CORE variants and will "just work"(tm). And because this asymmetry of CORE and non-CORE PT_REGS_PARM_SYSCALL definitions, your changes in v4 are a regression from the ones in v3 which were absolutely fine (I still don't get why you changed all of that, I've previously landed v3, which means it was 100% acceptable as is...), because v4 establishes more rigid relation between CORE and non-CORE variants. Anyways, let's get back to v3, drop UAPI changes, add struct pt_regs__s390x and whatever fields we need, use those with BPF_CORE_READ() and it should be ok with no ABI changes whatsoever.
On Tue, 2022-02-08 at 14:23 -0800, Andrii Nakryiko wrote: > On Tue, Feb 8, 2022 at 1:46 PM Ilya Leoshkevich <iii@linux.ibm.com> > wrote: > > > > On Tue, 2022-02-08 at 13:11 -0800, Alexei Starovoitov wrote: > > > On Tue, Feb 8, 2022 at 11:46 AM Ilya Leoshkevich > > > <iii@linux.ibm.com> > > > wrote: > > > > > > > > On Tue, 2022-02-08 at 11:25 -0800, Alexei Starovoitov wrote: > > > > > On Tue, Feb 08, 2022 at 06:16:35AM +0100, Ilya Leoshkevich > > > > > wrote: > > > > > > orig_x0's location is used by libbpf tracing macros, > > > > > > therefore > > > > > > it > > > > > > should not be moved. > > > > > > > > > > > > Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com> > > > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > > > > > --- > > > > > > arch/arm64/include/asm/ptrace.h | 4 ++++ > > > > > > 1 file changed, 4 insertions(+) > > > > > > > > > > > > diff --git a/arch/arm64/include/asm/ptrace.h > > > > > > b/arch/arm64/include/asm/ptrace.h > > > > > > index 41b332c054ab..7e34c3737839 100644 > > > > > > --- a/arch/arm64/include/asm/ptrace.h > > > > > > +++ b/arch/arm64/include/asm/ptrace.h > > > > > > @@ -185,6 +185,10 @@ struct pt_regs { > > > > > > u64 pstate; > > > > > > }; > > > > > > }; > > > > > > + /* > > > > > > + * orig_x0 is not exposed via struct user_pt_regs, > > > > > > but > > > > > > its > > > > > > location is > > > > > > + * assumed by libbpf's tracing macros, so it should > > > > > > not > > > > > > be > > > > > > moved. > > > > > > + */ > > > > > > > > > > In other words this comment is saying that the layout is ABI. > > > > > That's not the case. orig_x0 here and equivalent on s390 can > > > > > be > > > > > moved. > > > > > It will break bpf progs written without CO-RE and that is > > > > > expected. > > > > > Non CO-RE programs often do all kinds of > > > > > bpf_probe_read_kernel > > > > > and > > > > > will be breaking when kernel layout is changing. > > > > > I suggest to drop this patch and patch 12. > > > > > > > > Yeah, that was the intention here: to promote orig_x0 to ABI > > > > using > > > > a > > > > comment, since doing this by extending user_pt_regs turned out > > > > to > > > > be > > > > infeasible. I'm actually ok with not doing this, since programs > > > > compiled with kernel headers and using CO-RE macros will be > > > > fine. > > > > > > The comment like this doesn't convert kernel internal struct into > > > ABI. > > > The comment is just wrong. BPF progs access many kernel data > > > structs. > > > s390's and arm64's struct pr_regs is not special in that sense. > > > It's an internal struct. > > > > > > > As you say, we don't care about programs that don't use CO-RE > > > > too > > > > much > > > > here - if they break after an incompatible kernel change, fine. > > > > > > Before CO-RE was introduced bpf progs included kernel headers > > > and were breaking when kernel changes. Nothing new here. > > > See the history of bcc tools. Some of them are littered > > > with ifdef VERSION ==. > > > > > > > The question now is - how much do we care about programs that > > > > are > > > > > > > compiled with userspace headers? Andrii suggested to use > > > > offsetofend to > > > > make syscall macros work there, however, this now requires this > > > > ABI > > > > promotion. > > > > > > Today s390 and arm64 have user_pt_regs as a first field in > > > pt_regs. > > > That is kernel internal behavior and that part can change if arch > > > maintainers have a need for that. > > > bpf progs without CO-RE would have to be adjusted when kernel > > > changes. > > > Even with CO-RE it's ok to rename pt_regs->orig_gpr2 on s390. > > > The progs with CO-RE will break too. The authors of tracing bpf > > > progs > > > have to expect that sooner or later their progs will break and > > > they > > > would have to adjust them. > > > > When it comes to authors of tracing bpf progs, I agree that > > eventually > > they will have to adjust their code, CO-RE or not. However, in > > patch 13 > > I introduce the following libbpf macro: > > > > #if defined(__KERNEL__) || defined(__VMLINUX_H__) > > ... > > #else > > #define PT_REGS_PARM1_SYSCALL(x) \ > > (*(unsigned long *)(((char *)(x) + \ > > offsetofend(struct user_pt_regs, > > pstate)))) > > > > If we merge this series without freezing orig_x0's offset, in case > > of > > an incompatible kernel change the users of PT_REGS_PARMn_SYSCALL > > and > > BPF_KPROBE_SYSCALL, who build against userspace headers, will not > > simply have to update their code - they will have to upgrade > > libbpf. > > It's also not clear how PT_REGS_PARM1_SYSCALL in the upgraded > > libbpf > > will even look like, given that it would need to work on both old > > and > > new kernels. > > > > I've also briefly looked into MIPS' ptrace.h, and it looks as if > > their > > user_pt_regs has no relationship to kernel pt_regs. IIUC this means > > that it's not possible to correctly implement PT_REGS_PARMn_SYSCALL > > using MIPS userspace headers. > > > > So I wonder whether we should allow using PT_REGS_PARMn_SYSCALL and > > BPF_KPROBE_SYSCALL with userspace headers at all? Would it make > > sense > > to simply fail the compilation if PT_REGS_PARMn_SYSCALL is used > > without > > including kernel headers? > > Ok, my bad for suggesting those comments, I didn't realize the > consequences of making anything into a stable ABI. Let's not add any > comments, we don't need that. > > I think we should just come to terms that for some architectures this > syscall argument access won't work at least for some architectures > without CO-RE. For uniformity let's still have those > PT_REGS_PARM1_SYSCALL macro defined, but we should use > __bpf_unreachable or something like that to make sure it won't > compile > if someone tries to use it. Awesome, this would make quite a few headaches go away. > > But it's an entirely different story for CORE variants and there (as > I > explained on one of the previous patches) we can fabricate our own > definitions of pt_regs (architecture specific, using CO-RE struct > flavors) without any unnecessary assumptions about which include > headers the user is going to use. Hengqi's BPF_KPROBE_SYSCALL() macro > is always going to use CORE variants and will "just work"(tm). > > And because this asymmetry of CORE and non-CORE PT_REGS_PARM_SYSCALL > definitions, your changes in v4 are a regression from the ones in v3 > which were absolutely fine (I still don't get why you changed all of > that, I've previously landed v3, which means it was 100% acceptable > as > is...), because v4 establishes more rigid relation between CORE and > non-CORE variants. > > Anyways, let's get back to v3, drop UAPI changes, add struct > pt_regs__s390x and whatever fields we need, use those with > BPF_CORE_READ() and it should be ok with no ABI changes whatsoever. Ok.
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h index 41b332c054ab..7e34c3737839 100644 --- a/arch/arm64/include/asm/ptrace.h +++ b/arch/arm64/include/asm/ptrace.h @@ -185,6 +185,10 @@ struct pt_regs { u64 pstate; }; }; + /* + * orig_x0 is not exposed via struct user_pt_regs, but its location is + * assumed by libbpf's tracing macros, so it should not be moved. + */ u64 orig_x0; #ifdef __AARCH64EB__ u32 unused2;
orig_x0's location is used by libbpf tracing macros, therefore it should not be moved. Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- arch/arm64/include/asm/ptrace.h | 4 ++++ 1 file changed, 4 insertions(+)