mon: fix iterator mishandling in PGMap::apply_incremental

[ctheune]

Contribution Guidelines

• To sign and title your commits, please refer to Submitting Patches to Ceph.

• If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

Checklist

• Tracker (select at least one)
  • References tracker ticket
  • Very recent bug; references commit where it was introduced
  • New feature (ticket optional)
  • Doc update (no ticket needed)
  • Code cleanup (no ticket needed)
• Component impact
  • Affects Dashboard, opened tracker ticket
  • Affects Orchestrator, opened tracker ticket
  • No impact that needs to be tracked
• Documentation (select at least one)
  • Updates relevant documentation
  • No doc update is appropriate
• Tests (select at least one)
  • Includes unit test(s)
  • Includes integration test(s)
  • Includes bug reproducer
  • No tests

Show available Jenkins commands

• jenkins retest this please
• jenkins test classic perf
• jenkins test crimson perf
• jenkins test signed
• jenkins test make check
• jenkins test make check arm64
• jenkins test submodules
• jenkins test dashboard
• jenkins test dashboard cephadm
• jenkins test api
• jenkins test docs
• jenkins render docs
• jenkins test ceph-volume all
• jenkins test ceph-volume tox
• jenkins test windows

[ctheune]

This is a draft to wire it up with the issue tracker. Happy to fill in the template and clean up after discussion about this fix.

[rzarzynski]

From core PR scrub: this looks like a valid issue. We have a tracker for that?

[osnyx]

@rzarzynski Yes, they've been rather unnoticed so far. https://tracker.ceph.com/issues/58215 & https://tracker.ceph.com/issues/58303

[rzarzynski]

Thanks, @osnyx! I linked these tickets.

Would you mind to rework the commit description by:

1. putting the Fixes: https://tracker.ceph.com/issues/58303 line just before the Signed-off-by,
2. changing the title to: mon: fix iterator mishandling in PGMap::apply_incremental?

[rzarzynski]

Let's take a look on the pool_statfs member:

  typedef mempool::pgmap::map<
    std::pair<int64_t, int>,  // <pool, osd>
    store_statfs_t>
      per_osd_pool_statfs_t;

  per_osd_pool_statfs_t pool_statfs;

The underlying type is std::map. From the doc:

References and iterators to the erased elements are invalidated. Other references and iterators are not affected.

My understanding is that we shall not i++ if the i-pointed element has been erased. This stays coherent with the API design (returning new iterator) and even with the example there:

    // erase all odd numbers from c
    for (auto it = c.begin(); it != c.end();)
    {
        if (it->first % 2 != 0)
            it = c.erase(it);
        else
            ++it;
    }

[rzarzynski]

The core change LGTM! Putting DNM only b/c of the commit description.

[rzarzynski]

Let's take a look on the pool_statfs member:

  typedef mempool::pgmap::map<
    std::pair<int64_t, int>,  // <pool, osd>
    store_statfs_t>
      per_osd_pool_statfs_t;

  per_osd_pool_statfs_t pool_statfs;

The underlying type is std::map. From the doc:

References and iterators to the erased elements are invalidated. Other references and iterators are not affected.

My understanding is that we shall not i++ if the i-pointed element has been erased. This stays coherent with the API design (returning new iterator) and even with the example there:

    // erase all odd numbers from c
    for (auto it = c.begin(); it != c.end();)
    {
        if (it->first % 2 != 0)
            it = c.erase(it);
        else
            ++it;
    }

[osnyx]

@rzarzynski Requested changes are done. Thanks for taking care of this.

[rzarzynski]

 Failed to fetch http://ports.ubuntu.com/ubuntu-ports/pool/main/g/git/git_2.34.1-1ubuntu1.6_arm64.deb  Temporary failure resolving 'us.ports.ubuntu.com'

[rzarzynski]

jenkins retest this please

[rzarzynski]

Thanks for fixing this, @ctheune & @osnyx!

[neha-ojha]

jenkins test make check

[taodd]

Hit the same flaw code, see https://tracker.ceph.com/issues/61912.
In my case, the loop just keeps iterating on the invalid iterator without crashing.

[ctheune]

Thanks for fixing this, @ctheune & @osnyx!

Happy to! Glad this got picked up eventually - I was a bit worried that this didn't get any attention at all.

Getting it merged does increase motivation for future patches. ;)

[ljflores]

Hey @ctheune can you fill out the checklist in #49480 (comment)?

[ctheune]

Hey @ctheune can you fill out the checklist in #49480 (comment)?

Sure - done!