linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [GIT PULL] per signal_struct coredumps
@ 2021-11-03 19:07 Eric W. Biederman
  2021-11-03 19:32 ` pr-tracker-bot
  2021-11-03 19:34 ` Linus Torvalds
  0 siblings, 2 replies; 12+ messages in thread
From: Eric W. Biederman @ 2021-11-03 19:07 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: linux-kernel, Oleg Nesterov, Al Viro, Kees Cook, linux-api


Linus,

Please pull the per_signal_struct_coredumps-for-v5.16 branch from the git tree:

  git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git per_signal_struct_coredumps-for-v5.16

  HEAD: 3f66f86bfed33dee2e9c1d0e14486915bb0750b0 per signal_struct coredumps


Current coredumps are mixed up with the exit code, the signal handling
code, and the ptrace code making coredumps much more complicated than
necessary and difficult to follow.

This series of changes starts with ptrace_stop and cleans it up,
making it easier to follow what is happening in ptrace_stop.
Then cleans up the exec interactions with coredumps.
Then cleans up the coredump interactions with exit.
Then the coredump interactions with the signal handling code is cleaned
up.

The first and last changes are bug fixes for minor bugs.

I believe the fact that vfork followed by execve can kill the process
the called vfork if exec fails is sufficient justification to change
the userspace visible behavior.

In previous discussions some of these changes were organized differently
and individually appeared to make the code base worse.  As currently
written I believe they all stand on their own as cleanups and bug fixes.

Which means that even if the worst should happen and the last change
needs to be reverted for some unimaginable reason, the code base will
still be improved.

If the worst does not happen there are a more cleanups that can be made.
Signals that generate coredumps can easily become eligible for short
circuit delivery in complete_signal.  The entire rendezvous for
generating a coredump can move into get_signal.  The function
force_sig_info_to_task be written in a way that does not modify the
signal handling state of the target task (because coredumps are eligible
for short circuit delivery).  Many of these future cleanups can be done
another way but nothing so cleanly as if coredumps become per
signal_struct.

Eric W. Biederman (7):
      signal: Remove the bogus sigkill_pending in ptrace_stop
      ptrace: Remove the unnecessary arguments from arch_ptrace_stop
      exec: Check for a pending fatal signal instead of core_state
      exit: Factor coredump_exit_mm out of exit_mm
      coredump:  Don't perform any cleanups before dumping core
      coredump: Limit coredumps to a single thread group
      per signal_struct coredumps

 arch/ia64/include/asm/ptrace.h  |  4 +-
 arch/sparc/include/asm/ptrace.h |  8 ++--
 fs/binfmt_elf.c                 |  4 +-
 fs/binfmt_elf_fdpic.c           |  2 +-
 fs/coredump.c                   | 88 ++++++-----------------------------------
 fs/exec.c                       | 14 +++----
 fs/proc/array.c                 |  6 +--
 include/linux/mm_types.h        | 13 ------
 include/linux/ptrace.h          | 22 +++++------
 include/linux/sched.h           |  1 +
 include/linux/sched/signal.h    | 13 ++++++
 kernel/exit.c                   | 76 +++++++++++++++++++----------------
 kernel/fork.c                   |  4 +-
 kernel/signal.c                 | 49 ++++-------------------
 mm/debug.c                      |  4 +-
 mm/oom_kill.c                   |  6 +--
 16 files changed, 106 insertions(+), 208 deletions(-)

Link: https://lkml.kernel.org/r/87v92qx2c6.fsf@disp2133
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Eric

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [GIT PULL] per signal_struct coredumps
  2021-11-03 19:07 [GIT PULL] per signal_struct coredumps Eric W. Biederman
@ 2021-11-03 19:32 ` pr-tracker-bot
  2021-11-03 19:34 ` Linus Torvalds
  1 sibling, 0 replies; 12+ messages in thread
From: pr-tracker-bot @ 2021-11-03 19:32 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: Linus Torvalds, linux-kernel, Oleg Nesterov, Al Viro, Kees Cook,
	linux-api

The pull request you sent on Wed, 03 Nov 2021 14:07:36 -0500:

> git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git per_signal_struct_coredumps-for-v5.16

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/a602285ac11b019e9ce7c3907328e9f95f4967f0

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [GIT PULL] per signal_struct coredumps
  2021-11-03 19:07 [GIT PULL] per signal_struct coredumps Eric W. Biederman
  2021-11-03 19:32 ` pr-tracker-bot
@ 2021-11-03 19:34 ` Linus Torvalds
  2021-11-05 16:37   ` Eric W. Biederman
  1 sibling, 1 reply; 12+ messages in thread
From: Linus Torvalds @ 2021-11-03 19:34 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: Linux Kernel Mailing List, Oleg Nesterov, Al Viro, Kees Cook, Linux API

On Wed, Nov 3, 2021 at 12:07 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Please pull the per_signal_struct_coredumps-for-v5.16 branch

I've pulled it, but I'm not convinced about that odd extra merge
commit that contains the commentary.

That's what signed tags are for, and they have that explanation that
then makes it into the merge - plus they have the crypto signature to
show it all comes from you.

So that would have been the much better model than a fake extra merge.

But at least that extra merge did have explanations, so at least it
doesn't trigger me on _that_ level.

               Linus

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [GIT PULL] per signal_struct coredumps
  2021-11-03 19:34 ` Linus Torvalds
@ 2021-11-05 16:37   ` Eric W. Biederman
  2021-11-13 19:14     ` Linus Torvalds
  0 siblings, 1 reply; 12+ messages in thread
From: Eric W. Biederman @ 2021-11-05 16:37 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Linux Kernel Mailing List, Oleg Nesterov, Al Viro, Kees Cook, Linux API

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Wed, Nov 3, 2021 at 12:07 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>> Please pull the per_signal_struct_coredumps-for-v5.16 branch
>
> I've pulled it, but I'm not convinced about that odd extra merge
> commit that contains the commentary.
>
> That's what signed tags are for, and they have that explanation that
> then makes it into the merge - plus they have the crypto signature to
> show it all comes from you.
>
> So that would have been the much better model than a fake extra merge.
>
> But at least that extra merge did have explanations, so at least it
> doesn't trigger me on _that_ level.

I have been creating those when I place a patchset with an interesting
cover letter in a branch.  Now with the entire branch being just that
patchset, it doesn't make a lot of sense (except as somewhere to store
that cover letter so I don't loose it).  At other times when there are
multiple sets of changes on a single branch I think it makes more sense.

Am I missing a better way to preserve the cover letter for the
changes when multiple sets of changes land in a single branch?

Eric

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [GIT PULL] per signal_struct coredumps
  2021-11-05 16:37   ` Eric W. Biederman
@ 2021-11-13 19:14     ` Linus Torvalds
  2021-11-14  6:32       ` Junio C Hamano
  0 siblings, 1 reply; 12+ messages in thread
From: Linus Torvalds @ 2021-11-13 19:14 UTC (permalink / raw)
  To: Eric W. Biederman, Git List Mailing
  Cc: Linux Kernel Mailing List, Oleg Nesterov, Al Viro, Kees Cook, Linux API

[ Coming back to this email because I'm starting to see the light at
the end of the merge window tunnel .. ]

Adding the git list to see if somebody has suggestions, and
top-posting because the email from Eric is more of "explanation for
the issue" than anything else, so I'm quoting it at the end for
context.

The basic issue is how to sanely keep track of a cover letter when you
have a branch that you haven't sent out yet, but will ask somebody to
pull. It may still be seeing more testing and development before that
pull happens, though.

This very much smells of what the "branch description" is all about, but

 (a) I suspect "git branch --edit-description" is not very well known

 (b) it works well with "git request-pull", but not so much some other
things (like copying it into a signed tag)

 (c) it makes an unholy mess of your config file if you actually use
it for extensive explanations (branch descriptions _work_ for
multi-line messages, but it really was designed as a one-liner thing).

 (d) it doesn't work across repositories (ie multiple developers or
even just a single developer on multiple machines).

IOW, the "branch description" is _kind_ of the right thing, but not really.

The fake merge _does_ solve all these issues, it just then ends up
leaving that turd around in the history.

An empty commit would do it as well, but an empty commit very easily
gets lost (git rebase etc). The fake merge does have similar issues.

Both a fake merge, and an empty commit have the advantage that they
are easy to see and work with (ie "git log" and all the other git
workflows work very naturally).

Comments from git people?

                Linus

On Fri, Nov 5, 2021 at 9:38 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
> > On Wed, Nov 3, 2021 at 12:07 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> >
> > I've pulled it, but I'm not convinced about that odd extra merge
> > commit that contains the commentary.
> >
> > That's what signed tags are for, and they have that explanation that
> > then makes it into the merge - plus they have the crypto signature to
> > show it all comes from you.
> >
> > So that would have been the much better model than a fake extra merge.
> >
> > But at least that extra merge did have explanations, so at least it
> > doesn't trigger me on _that_ level.
>
> I have been creating those when I place a patchset with an interesting
> cover letter in a branch.  Now with the entire branch being just that
> patchset, it doesn't make a lot of sense (except as somewhere to store
> that cover letter so I don't loose it).  At other times when there are
> multiple sets of changes on a single branch I think it makes more sense.
>
> Am I missing a better way to preserve the cover letter for the
> changes when multiple sets of changes land in a single branch?
>
> Eric

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [GIT PULL] per signal_struct coredumps
  2021-11-13 19:14     ` Linus Torvalds
@ 2021-11-14  6:32       ` Junio C Hamano
  2021-11-14  9:36         ` Ævar Arnfjörð Bjarmason
  2021-11-14 17:16         ` Eric W. Biederman
  0 siblings, 2 replies; 12+ messages in thread
From: Junio C Hamano @ 2021-11-14  6:32 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Eric W. Biederman, Git List Mailing, Linux Kernel Mailing List,
	Oleg Nesterov, Al Viro, Kees Cook, Linux API

Linus Torvalds <torvalds@linux-foundation.org> writes:

> The basic issue is how to sanely keep track of a cover letter when you
> have a branch that you haven't sent out yet, but will ask somebody to
> pull. It may still be seeing more testing and development before that
> pull happens, though.
>
> This very much smells of what the "branch description" is all about, but
>
>  (a) I suspect "git branch --edit-description" is not very well known

True.


>  (b) it works well with "git request-pull", but not so much some other
> things (like copying it into a signed tag)

I think that is just a matter of programming ;-)

>  (c) it makes an unholy mess of your config file if you actually use
> it for extensive explanations (branch descriptions _work_ for
> multi-line messages, but it really was designed as a one-liner thing).

Not, really.

The "-m" option similar to "commit/tag" is deliberately omitted and
use of editor is forced, to encourage better than one-liner
information.  cf. b7200e83 (branch: teach --edit-description option,
2011-09-20).

The unholy mess is true if you are in the habit of editing .git/config
in your editor, but that is to be expected if you are storing multi
paragraph description as a value of a configuration variable.

>  (d) it doesn't work across repositories (ie multiple developers or
> even just a single developer on multiple machines).

This is the biggest issue.

> IOW, the "branch description" is _kind_ of the right thing, but not really.

Having said all that, quite honestly, as the inventor of the
"--edit-description", I did it as sort of a joke, and not a serious
"feature".

> An empty commit would do it as well, but an empty commit very easily
> gets lost (git rebase etc). The fake merge does have similar issues.

These days, I think rebase distinguishes between "an empty commit
that is deliberately empty from the beginning" and "a commit that
was not empty, but because we are applying on a new base, it has
become unnecessary and empty", and we can tell the command to drop
the latter while keeping the former.  So if I were to design a
recommended workflow (and add any missing workflow elements), it
would be:

 - You develop your N-patch series on a branch;

 - You conclude with an empty commit that records your cover letter
   material.

 - "git commit" may want to learn a new option that automatically
   prepares summary of the last N patches in the commit log
   editor, and the option should imply the "--allow-empty" option.
   That would help when editing such an empty commit that will
   become the cover letter.

 - You repeatedly "rebase -i", "cherry-pick", etc. to whip your
   branch into shape.

 - You can push and fetch such a branch among your machines and your
   group.

 - "git format-patch" may want to recognize that the topmost commit
   is an empty commit, and use that as the seed material for the
   cover letter.

 - If your project's pull request requires a signed tag with cover
   letter material, "git tag -s" may want to learn a new option to
   be fed such a branch with N-patch plus the topmost empty commit,
   and tag the last real commit in the topic (i.e. the parent of the
   topmost empty commit) with material taken from the topmost empty
   commit.

> Both a fake merge, and an empty commit have the advantage that they
> are easy to see and work with (ie "git log" and all the other git
> workflows work very naturally).
>
> Comments from git people?
>
>                 Linus

I can see how the above outline would work.  I do not know if it
would work well for your project, or there are other workflows for
which the above outline would be insufficient.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [GIT PULL] per signal_struct coredumps
  2021-11-14  6:32       ` Junio C Hamano
@ 2021-11-14  9:36         ` Ævar Arnfjörð Bjarmason
  2021-11-14 17:16         ` Eric W. Biederman
  1 sibling, 0 replies; 12+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-14  9:36 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Linus Torvalds, Eric W. Biederman, Git List Mailing,
	Linux Kernel Mailing List, Oleg Nesterov, Al Viro, Kees Cook,
	Linux API


On Sat, Nov 13 2021, Junio C Hamano wrote:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> The basic issue is how to sanely keep track of a cover letter when you
>> have a branch that you haven't sent out yet, but will ask somebody to
>> pull. It may still be seeing more testing and development before that
>> pull happens, though.
>>
>> This very much smells of what the "branch description" is all about, but
>>
>>  (a) I suspect "git branch --edit-description" is not very well known
>
> True.
>
>
>>  (b) it works well with "git request-pull", but not so much some other
>> things (like copying it into a signed tag)
>
> I think that is just a matter of programming ;-)
>
>>  (c) it makes an unholy mess of your config file if you actually use
>> it for extensive explanations (branch descriptions _work_ for
>> multi-line messages, but it really was designed as a one-liner thing).
>
> Not, really.
>
> The "-m" option similar to "commit/tag" is deliberately omitted and
> use of editor is forced, to encourage better than one-liner
> information.  cf. b7200e83 (branch: teach --edit-description option,
> 2011-09-20).
>
> The unholy mess is true if you are in the habit of editing .git/config
> in your editor, but that is to be expected if you are storing multi
> paragraph description as a value of a configuration variable.
>
>>  (d) it doesn't work across repositories (ie multiple developers or
>> even just a single developer on multiple machines).
>
> This is the biggest issue.
>
>> IOW, the "branch description" is _kind_ of the right thing, but not really.
>
> Having said all that, quite honestly, as the inventor of the
> "--edit-description", I did it as sort of a joke, and not a serious
> "feature".
>
>> An empty commit would do it as well, but an empty commit very easily
>> gets lost (git rebase etc). The fake merge does have similar issues.
>
> These days, I think rebase distinguishes between "an empty commit
> that is deliberately empty from the beginning" and "a commit that
> was not empty, but because we are applying on a new base, it has
> become unnecessary and empty", and we can tell the command to drop
> the latter while keeping the former.  So if I were to design a
> recommended workflow (and add any missing workflow elements), it
> would be:
>
>  - You develop your N-patch series on a branch;
>
>  - You conclude with an empty commit that records your cover letter
>    material.
>
>  - "git commit" may want to learn a new option that automatically
>    prepares summary of the last N patches in the commit log
>    editor, and the option should imply the "--allow-empty" option.
>    That would help when editing such an empty commit that will
>    become the cover letter.
>
>  - You repeatedly "rebase -i", "cherry-pick", etc. to whip your
>    branch into shape.
>
>  - You can push and fetch such a branch among your machines and your
>    group.
>
>  - "git format-patch" may want to recognize that the topmost commit
>    is an empty commit, and use that as the seed material for the
>    cover letter.
>
>  - If your project's pull request requires a signed tag with cover
>    letter material, "git tag -s" may want to learn a new option to
>    be fed such a branch with N-patch plus the topmost empty commit,
>    and tag the last real commit in the topic (i.e. the parent of the
>    topmost empty commit) with material taken from the topmost empty
>    commit.
>
>> Both a fake merge, and an empty commit have the advantage that they
>> are easy to see and work with (ie "git log" and all the other git
>> workflows work very naturally).
>>
>> Comments from git people?
>>
>>                 Linus
>
> I can see how the above outline would work.  I do not know if it
> would work well for your project, or there are other workflows for
> which the above outline would be insufficient.

There's a more general question to be considered here which is how the
object model is supposed to keep track of this sort of thing, if at all.

One solution is (dare I say?) "git notes". I.e. if you're keeping a
notes/CL about a set of commits you could publish a notes branch where
the CL is by convention on the tip of that commit. Now when someone else
"takes over" your branch they'll "git fetch" your notes, have set up
approprite core.{notes,display}Ref, copy your notes into their own note,
then edit/amend/publish/push.

Setting that up should be small matter of scripting around existing
commands, and should be an improvement on having to manually share or
extract what's now stored in branch descriptions.

On the other hand that information is rather unwieldy to deal with, and
"notes" support isn't very widespread on e.g. git hosting sites
(althougt that's also a chicken & egg problem).

Another solution that fits nicely into the data model is to consider
this the same "problem" as PGP signed commits. I.e. an an
"x-metadata-cover-letter" header could be added to the tip commit object
itself, either an --allow-empty --allow-empty-message commit, or "git
merge --no-ff" in case the range that CL covers is ambiguous (which in
that case would start with the merge base of the two parents).

If git's going to have some native-ish solution to this problem it
should also be considered that what E-Mail based workflows want to do
with CL's is analogous to what popular hosting sites are doing with the
text field for a PR/MR. I.e. ideally such a cover letter could be
stored/structured in such a way that if you pushed a branch to open such
a PR/MR the "cover letter" could be extracted.

Which also gets into questions of if that data should be stored
permanently. I.e. it's a merge commit as described above would the
person merging it expect to merge down the parent (i.e. sans
coverletter), or if it's stored in the commit object rewrite the commit?

And for some text-only cover letters might suffice, but git users in
general observably upload attachments to explain their commits (say a
screenshot of performance numbers). So a design that combined the above
with the ability to point at a tree might be preferred.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [GIT PULL] per signal_struct coredumps
  2021-11-14  6:32       ` Junio C Hamano
  2021-11-14  9:36         ` Ævar Arnfjörð Bjarmason
@ 2021-11-14 17:16         ` Eric W. Biederman
  2021-11-16  6:49           ` Junio C Hamano
  2021-11-16  8:29           ` Junio C Hamano
  1 sibling, 2 replies; 12+ messages in thread
From: Eric W. Biederman @ 2021-11-14 17:16 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Linus Torvalds, Git List Mailing, Linux Kernel Mailing List,
	Oleg Nesterov, Al Viro, Kees Cook, Linux API

Junio C Hamano <gitster@pobox.com> writes:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> The basic issue is how to sanely keep track of a cover letter when you
>> have a branch that you haven't sent out yet, but will ask somebody to
>> pull. It may still be seeing more testing and development before that
>> pull happens, though.
>>
>> This very much smells of what the "branch description" is all about, but
>>
>>  (a) I suspect "git branch --edit-description" is not very well known
>
> True.
>
>
>>  (b) it works well with "git request-pull", but not so much some other
>> things (like copying it into a signed tag)
>
> I think that is just a matter of programming ;-)
>
>>  (c) it makes an unholy mess of your config file if you actually use
>> it for extensive explanations (branch descriptions _work_ for
>> multi-line messages, but it really was designed as a one-liner thing).
>
> Not, really.
>
> The "-m" option similar to "commit/tag" is deliberately omitted and
> use of editor is forced, to encourage better than one-liner
> information.  cf. b7200e83 (branch: teach --edit-description option,
> 2011-09-20).
>
> The unholy mess is true if you are in the habit of editing .git/config
> in your editor, but that is to be expected if you are storing multi
> paragraph description as a value of a configuration variable.
>
>>  (d) it doesn't work across repositories (ie multiple developers or
>> even just a single developer on multiple machines).
>
> This is the biggest issue.
>
>> IOW, the "branch description" is _kind_ of the right thing, but not really.
>
> Having said all that, quite honestly, as the inventor of the
> "--edit-description", I did it as sort of a joke, and not a serious
> "feature".
>
>> An empty commit would do it as well, but an empty commit very easily
>> gets lost (git rebase etc). The fake merge does have similar issues.
>
> These days, I think rebase distinguishes between "an empty commit
> that is deliberately empty from the beginning" and "a commit that
> was not empty, but because we are applying on a new base, it has
> become unnecessary and empty", and we can tell the command to drop
> the latter while keeping the former.  So if I were to design a
> recommended workflow (and add any missing workflow elements), it
> would be:
>
>  - You develop your N-patch series on a branch;
>
>  - You conclude with an empty commit that records your cover letter
>    material.
>
>  - "git commit" may want to learn a new option that automatically
>    prepares summary of the last N patches in the commit log
>    editor, and the option should imply the "--allow-empty" option.
>    That would help when editing such an empty commit that will
>    become the cover letter.
>
>  - You repeatedly "rebase -i", "cherry-pick", etc. to whip your
>    branch into shape.
>
>  - You can push and fetch such a branch among your machines and your
>    group.
>
>  - "git format-patch" may want to recognize that the topmost commit
>    is an empty commit, and use that as the seed material for the
>    cover letter.
>
>  - If your project's pull request requires a signed tag with cover
>    letter material, "git tag -s" may want to learn a new option to
>    be fed such a branch with N-patch plus the topmost empty commit,
>    and tag the last real commit in the topic (i.e. the parent of the
>    topmost empty commit) with material taken from the topmost empty
>    commit.

I think an empty commit at the top of a development branch sounds
like a nice place to store the cover letter for a set of changes.
That is a related problem but that is not the problem that inspired
the no-op merge commits.



I have not seen addressed the workflow that actually inspired this
odd thing I am doing.  So let me see if I can describe the problem
that inspired the merge commit more clearly.

Before the merge window for v5.17 I expect to be working on
a topic I will loosely call "do_exit_coredumps_and_signals".

There are going to be several changesets (something like):
"Move coredumps rendezvous into get_signal"
"Use the same exit code in all implementations of die"
"Use signal short circuit delivery for coredumps"
"Use signal short circuit delivery whenever possible"
"Replace do_exit with a different helper for use by die"

Each of those will consist of 5-10 patches and need to be individually
reviewed and depend upon each other.  In the roughly 2 months of
development time before v5.17 I can expect to get several of those
changesets.  Each changeset will depend upon the work of the changeset
before.

As each changeset is reviewed and finalized I expect I will put it on
the topic branch with a merge commit containing the description letter.
That merge commit will contain a "Link:" tag to the posting on the
mailing list so that people can find the full description.

When put into the topic branch after review the commits are frozen
and ready to be sent to Linus for merging, when the next merge window
opens.




When the development window closes and the merge window opens I will run
"git shortlog" see what is there and write up a description for the
entire topic branch.  Ideally I will put that into a signed tag etc
before I send it to Linus.

In the case that triggered this conversation I happened to only have a
single changeset with a single merge commit in the topic branch which
looks very odd, but that is mot definitely not the case I want to
optimize for.


The changes I am making are digging through some old grotty areas of the
kernel.  It isn't uncommon for me to be fixing issues that predate git.
So it takes some sleuthing and description so that other people can see
what is going on in each of those changes.  So I want to preserve the
description of what and what is going on with those changesets as
much as possible.

A big rollup into a single description for Linus of the entire topic
branch is necessary, but will generally not contain all of the detail of
the cover letters for the patch sets.

I need something like a merge commit rather than an empty commit so
that both the start and the end of the series of changes is shown.

Notes probably don't work because I want the description of what is
going to persist and be available to "git log".  So that the next poor
sucker who touches the code can have a change of figuring out what
I was thinking.

If I have just one fake-merge I would not mind if request-pull or
something incorporated it, but with several I am most definitely going
to need to edit the message so it is at the 10,000 foot view of the
entire topic branch, and not at the nitty gritty view of a single patch
set.


There was only a single changeset in the branch that caused this because
I am making what is remotely possibly a breaking change and I need to
get it into a released code base to verify that I am not breaking
anyone.  If I don't need to revert that change then there are very
substantial cleanups I can make in the kernel.   So all of the
additional changesets have to wait until v5.16 is released before I can
ask Linus to pull them.

Which meant during this one development cycle I was not able to build
one changeset on top of another changeset because one of the changes is
something of a risk.  That is a degenerate case of my normal expected
work-flow.


Eric

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [GIT PULL] per signal_struct coredumps
  2021-11-14 17:16         ` Eric W. Biederman
@ 2021-11-16  6:49           ` Junio C Hamano
  2021-11-16  8:29           ` Junio C Hamano
  1 sibling, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2021-11-16  6:49 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: Linus Torvalds, Git List Mailing, Linux Kernel Mailing List,
	Oleg Nesterov, Al Viro, Kees Cook, Linux API

ebiederm@xmission.com (Eric W. Biederman) writes:

> I have not seen addressed the workflow that actually inspired this
> odd thing I am doing.  So let me see if I can describe the problem
> that inspired the merge commit more clearly.

Where can I learn more about the "fake" merge commit (assume I do
not know anything more than what Linus wrote in the message to the
git mailing list)?  Even after re-reading your description twice,
I get that you are using an extra merge commit as a place to store
the cover letter material, I am guessing that one of the parents
(which one???) is the tip of the finished "changeset" (I take the
word to mean "one of more commits on the same theme, in a single
strand of pearls"), but I am not sure what the other parent is to
make that a "merge".  If it is "fake", I guess that any random point
in Linus's history would do, but I can understand that the maintainer
would complain about such a seemingly unnecessary (back) merge.

> Before the merge window for v5.17 I expect to be working on
> a topic I will loosely call "do_exit_coredumps_and_signals".
>
> There are going to be several changesets (something like):
> "Move coredumps rendezvous into get_signal"
> "Use the same exit code in all implementations of die"
> "Use signal short circuit delivery for coredumps"
> "Use signal short circuit delivery whenever possible"
> "Replace do_exit with a different helper for use by die"
>
> Each of those will consist of 5-10 patches and need to be individually
> reviewed and depend upon each other.  In the roughly 2 months of
> development time before v5.17 I can expect to get several of those
> changesets.  Each changeset will depend upon the work of the changeset
> before.

Up to this point, I think I get what is going on.  You've worked
together with your reviewer and came up several patches to achieve
the "Move coredumps rendezvous into get_signal" task, so now you
have one topic branch that forks from some point in Linus's history
and houses these patches.  If "Use the same exit code" topic builds
on the "Move coredumps" one, the topic branch for the former may
fork from the tip of the latter.

> As each changeset is reviewed and finalized I expect I will put it on
> the topic branch with a merge commit containing the description letter.

This part I do not understand.  What is merged into what?  The tip
of the "Move coredumps" series of commits gets merged into the
previous stable release from Linus or some appropriate point inhis
history, and the topic branch points at that merge?

> That merge commit will contain a "Link:" tag to the posting on the
> mailing list so that people can find the full description.

If a signed tag were used to store that description and Link,
wouldn't that be sufficient?  Then Linus would pull that signed tag
to see it is from you, the merge would show which tag was merged and
what the tag said.  And we do not see the fake merge---I think the
maintainer's complaint and the problem the fake merge brings into
the history is that, while one of its parents, namely, the tip of
the "Move coredumps" series of commits, does have meaning, the other
parent does not.  It can be any ancient commit.

It _might_ make it a bit more palatable to Linus if the merit of
using a merge includes that the other parent can record the fork
point of the main series of commits (i.e. the merge will have to be
created with "merge --no-ff"), but I still feel that the argument is
weak, when a signed tag would work better (and I probably am missing
the use cases in which fake merges work better than signed tags).

> When put into the topic branch after review the commits are frozen
> and ready to be sent to Linus for merging, when the next merge window
> opens.
>
> When the development window closes and the merge window opens I will run
> "git shortlog" see what is there and write up a description for the
> entire topic branch.  Ideally I will put that into a signed tag etc
> before I send it to Linus.

Here, what do you exactly mean by "the entire topic branch"?  For a
single "changeset" like "Move coredumps", or all of the changesets?

Assuming that the answer is the former, and assuming that the "topic
branch" for "Move coredumps" look like:

 a. it forks from some point in the upstream history;

 b. it build one or more commits on it, a single strand of pearls;

 c. it is capped with a (fake) merge to merge the above into some
    point in the upstream history;

what is missing from the Git toolset to turn the above two into a
signed tag that points at the tip of b. (i.e. without the fake
merge)?  Ideally, after b. is made, don't you want to go directly to
a signed tag, instead of a (fake) merge?

> In the case that triggered this conversation I happened to only have a
> single changeset with a single merge commit in the topic branch which
> looks very odd, but that is mot definitely not the case I want to
> optimize for.

It is unclear to me why the number of commits in the b. part
matters.  Be it one or 40, the (fake) merge at the tip seems
unnecessary, when the whole thing is merged into the upstream
history.

I seriously doubt that I am getting the whole requirement, as it
seems that "develop a series of commits on a branch, that may or may
not be dependent with each other, and ask the branch to be pulled by
giving the description in a signed tag that points at the tip of the
work" should work OK?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [GIT PULL] per signal_struct coredumps
  2021-11-14 17:16         ` Eric W. Biederman
  2021-11-16  6:49           ` Junio C Hamano
@ 2021-11-16  8:29           ` Junio C Hamano
  2021-11-16 15:14             ` Eric W. Biederman
  2021-11-18  9:29             ` Junio C Hamano
  1 sibling, 2 replies; 12+ messages in thread
From: Junio C Hamano @ 2021-11-16  8:29 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: Linus Torvalds, Git List Mailing, Linux Kernel Mailing List,
	Oleg Nesterov, Al Viro, Kees Cook, Linux API

Junio C Hamano <gitster@pobox.com> writes:

> make that a "merge".  If it is "fake", I guess that any random point
> in Linus's history would do, but I can understand that the maintainer
> would complain about such a seemingly unnecessary (back) merge.

Having thought about it a bit more, I am not sure if these merges
are truly "fake", or just a normal part of distributed development.

As a degenerated case, first I'd imagine you have a patch series
that focuses on a single "theme".  You perfect the patches, you fork
a topic branch from an appropriate "public" commit of your upstream
(e.g. the last stable release from Linus), you add a signed tag at
the tip of that topic branch, and you ask a (subsystem) maintainer
to pull from you.  The subsystem maintainer's tree will have series
of merges to collect work from other people working in the subsystem
('x'), and the pull from you will create a merge whose first parent
is one of these 'x' (i.e. the work by the maintainer so far), and
the second parent of it is the tip of your work.  The merge commit M
gives a detailed description of what happend on the side branch and
its mergetag header carries the contents of the tag you created for
the pull request.

      \   \
    ---x---x---M
              / Subsystem maintainer pulls from you
             /
  ...---o---o (your work)

Your next topic, which is a chunk of the same larger theme, may
depend on what you did in the commits in this initial series 'o'.


      \   \       \   \
    ---x---x---M---x---x---N
              /           / Subsystem maintainer pulls from you again
             /           /
  ...---o---o---p---p---p (your second batch)


Eventually, this will be pulled into Linus's tree when the subsystem
maintainer is ready to send the whole thing.

                              Y--- (Linus's tree)
                             / Linus pulls from subsystem maintainer
      \   \       \   \     /
    ---x---x---M---x---x---N (Subsystem maintainer's tree)
              /           /
             /           /
  ...---o---o---p---p---p (Your tree)

The above picture only depicts two topics, one directly building on
top of the other, from you, but that is simplified merely for
illustration purposes.  The real history may have more topics, some
are dependent on others, while some are independent.

Now, if you have many related but more or less independent topic
branches that will support a larger theme, it would be quite natural
if you acted as your own "subsystem" maintainer, in other words, in
the above picture:

 . you are in control of not just the bottom line, but in the middle
   line of development;

 . you do not have 'x' that merges from other people;

 . but you do have M and N, and use these merges just like a
   subsystem maintainer would use to describe the work done in the
   side branches.

and offer 'N' as the tip of a "larger" topic that has internal
structure, not just a single strand of pearls, by adding a signed
tag on 'N' and throwing a pull request at Linus (or whoever is
immediately above your level).

Is that what happened (as I said, I lack context)?  If so, I do not
see much problem in the situation.  But this assumes that these so
called "fake" merges are merging into right first parents.



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [GIT PULL] per signal_struct coredumps
  2021-11-16  8:29           ` Junio C Hamano
@ 2021-11-16 15:14             ` Eric W. Biederman
  2021-11-18  9:29             ` Junio C Hamano
  1 sibling, 0 replies; 12+ messages in thread
From: Eric W. Biederman @ 2021-11-16 15:14 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Linus Torvalds, Git List Mailing, Linux Kernel Mailing List,
	Oleg Nesterov, Al Viro, Kees Cook, Linux API

Junio C Hamano <junio@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> make that a "merge".  If it is "fake", I guess that any random point
>> in Linus's history would do, but I can understand that the maintainer
>> would complain about such a seemingly unnecessary (back) merge.
>
> Having thought about it a bit more, I am not sure if these merges
> are truly "fake", or just a normal part of distributed development.
>
> As a degenerated case, first I'd imagine you have a patch series
> that focuses on a single "theme".  You perfect the patches, you fork
> a topic branch from an appropriate "public" commit of your upstream
> (e.g. the last stable release from Linus), you add a signed tag at
> the tip of that topic branch, and you ask a (subsystem) maintainer
> to pull from you.  The subsystem maintainer's tree will have series
> of merges to collect work from other people working in the subsystem
> ('x'), and the pull from you will create a merge whose first parent
> is one of these 'x' (i.e. the work by the maintainer so far), and
> the second parent of it is the tip of your work.  The merge commit M
> gives a detailed description of what happend on the side branch and
> its mergetag header carries the contents of the tag you created for
> the pull request.
>
>       \   \
>     ---x---x---M
>               / Subsystem maintainer pulls from you
>              /
>   ...---o---o (your work)
>
> Your next topic, which is a chunk of the same larger theme, may
> depend on what you did in the commits in this initial series 'o'.
>
>
>       \   \       \   \
>     ---x---x---M---x---x---N
>               /           / Subsystem maintainer pulls from you again
>              /           /
>   ...---o---o---p---p---p (your second batch)
>
>
> Eventually, this will be pulled into Linus's tree when the subsystem
> maintainer is ready to send the whole thing.
>
>                               Y--- (Linus's tree)
>                              / Linus pulls from subsystem maintainer
>       \   \       \   \     /
>     ---x---x---M---x---x---N (Subsystem maintainer's tree)
>               /           /
>              /           /
>   ...---o---o---p---p---p (Your tree)
>
> The above picture only depicts two topics, one directly building on
> top of the other, from you, but that is simplified merely for
> illustration purposes.  The real history may have more topics, some
> are dependent on others, while some are independent.
>
> Now, if you have many related but more or less independent topic
> branches that will support a larger theme, it would be quite natural
> if you acted as your own "subsystem" maintainer, in other words, in
> the above picture:
>
>  . you are in control of not just the bottom line, but in the middle
>    line of development;
>
>  . you do not have 'x' that merges from other people;
>
>  . but you do have M and N, and use these merges just like a
>    subsystem maintainer would use to describe the work done in the
>    side branches.
>
> and offer 'N' as the tip of a "larger" topic that has internal
> structure, not just a single strand of pearls, by adding a signed
> tag on 'N' and throwing a pull request at Linus (or whoever is
> immediately above your level).
>
> Is that what happened (as I said, I lack context)?  If so, I do not
> see much problem in the situation.  But this assumes that these so
> called "fake" merges are merging into right first parents.

Yes.  I write and post the patches with my developer hat on,
and I merge them with my maintainer hat on, then ultimately I send
them to Linus with the same maintainer hat on.


The full email conversation is at:
https://lore.kernel.org/all/878ry512iv.fsf@disp2133/T/#u

Here is where Linus merged the change:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a602285ac11b019e9ce7c3907328e9f95f4967f0

In this specific case it is a very degenerate case as there was only one
set of changes.


The one difference from my work flow and the one you described
is that I haven't reach the point of signing my pull requests.


In general and especially this cycle I intend to have multiple
changesets each with their own merge commit delineating them.  Short of
being informed of a better way to work.


I suspect the conversation is simply because the pull request was
sufficiently degenerate that things just looked really weird.  But I am
open to learning otherwise.

Eric





^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [GIT PULL] per signal_struct coredumps
  2021-11-16  8:29           ` Junio C Hamano
  2021-11-16 15:14             ` Eric W. Biederman
@ 2021-11-18  9:29             ` Junio C Hamano
  1 sibling, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2021-11-18  9:29 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Eric W. Biederman, Linus Torvalds, Git List Mailing,
	Linux Kernel Mailing List, Oleg Nesterov, Al Viro, Kees Cook,
	Linux API

Junio C Hamano <junio@pobox.com> writes:

>                               Y--- (Linus's tree)
>                              / Linus pulls from subsystem maintainer
>       \   \       \   \     /
>     ---x---x---M---x---x---N (Subsystem maintainer's tree)
>               /           /
>              /           /
>   ...---o---o---p---p---p (Your tree)
>
> The above picture only depicts two topics, one directly building on
> top of the other, from you, but that is simplified merely for
> illustration purposes.  The real history may have more topics, some
> are dependent on others, while some are independent.
>
> Now, if you have many related but more or less independent topic
> branches that will support a larger theme, it would be quite natural
> if you acted as your own "subsystem" maintainer, in other words, in
> ...
> and offer 'N' as the tip of a "larger" topic that has internal
> structure, not just a single strand of pearls, by adding a signed
> tag on 'N' and throwing a pull request at Linus (or whoever is
> immediately above your level).
> 
> Is that what happened (as I said, I lack context)?  If so, I do not
> see much problem in the situation.  But this assumes that these so
> called "fake" merges are merging into right first parents.

Addendum.

If you have only one topic (i.e. you do not have o-o and p-p-p in
the above picture, but just o-o), then it would be quite strange to
create M and offer it to the upstream, as M's first parent, as well
as the bottom of the o-o chain, would be something the upstream has
and the merge would look redundant from upstream's point of view, as
they will be creating another merge of their tip and M, at which
point they'd rather merge the topmost commit in the o-o chain
directly without having to deal with M.


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-11-18  9:29 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-03 19:07 [GIT PULL] per signal_struct coredumps Eric W. Biederman
2021-11-03 19:32 ` pr-tracker-bot
2021-11-03 19:34 ` Linus Torvalds
2021-11-05 16:37   ` Eric W. Biederman
2021-11-13 19:14     ` Linus Torvalds
2021-11-14  6:32       ` Junio C Hamano
2021-11-14  9:36         ` Ævar Arnfjörð Bjarmason
2021-11-14 17:16         ` Eric W. Biederman
2021-11-16  6:49           ` Junio C Hamano
2021-11-16  8:29           ` Junio C Hamano
2021-11-16 15:14             ` Eric W. Biederman
2021-11-18  9:29             ` Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).