Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Roman Gushchin <guro@fb.com>
To:     Alexei Starovoitov <ast@fb.com>
CC:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Tejun Heo <tj@kernel.org>,
        "bpf@vger.kernel.org" <bpf@vger.kernel.org>,
        Kernel Team <Kernel-team@fb.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH bpf-next] bpf: fix cgroup bpf release synchronization
Thread-Topic: [PATCH bpf-next] bpf: fix cgroup bpf release synchronization
Thread-Index: AQHVKjTees07LGtTbkiqSncwtVU7j6aqJZyAgAAJMAA=
Date:   Mon, 24 Jun 2019 04:02:20 +0000
Message-ID: <20190624040211.GA10696@castle.dhcp.thefacebook.com>
References: <20190624023051.4168487-1-guro@fb.com>
 <91017042-1b59-6110-dfdd-13cfbbec1ae1@fb.com>
In-Reply-To: <91017042-1b59-6110-dfdd-13cfbbec1ae1@fb.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-messagesentrepresentingtype: 1
x-originating-ip: [2620:10d:c090:180::1:4969]
x-ms-publictraffictype: Email
x-ms-office365-filtering-correlation-id: 83837ae1-4780-4444-d99e-08d6f858c153
x-microsoft-antispam: BCL:0;PCL:0;RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600148)(711020)(4605104)(1401327)(2017052603328)(7193020);SRVR:BN8PR15MB3202;
x-ms-traffictypediagnostic: BN8PR15MB3202:
x-microsoft-antispam-prvs: <BN8PR15MB32028BC9EEF6D3F5F62EFD9DBEE00@BN8PR15MB3202.namprd15.prod.outlook.com>
x-ms-oob-tlc-oobclassifiers: OLM:3513;
x-forefront-prvs: 007814487B
x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(39860400002)(136003)(366004)(396003)(346002)(376002)(189003)(199004)(71190400001)(71200400001)(53546011)(6506007)(8936002)(386003)(102836004)(81166006)(81156014)(186003)(229853002)(52116002)(6636002)(6436002)(86362001)(76176011)(33656002)(2906002)(6116002)(256004)(99286004)(6486002)(8676002)(11346002)(446003)(5024004)(14444005)(1076003)(6246003)(53936002)(73956011)(66946007)(46003)(478600001)(4326008)(54906003)(64756008)(66446008)(25786009)(6512007)(9686003)(66476007)(66556008)(7736002)(5660300002)(6862004)(316002)(68736007)(305945005)(486006)(14454004)(476003);DIR:OUT;SFP:1102;SCL:1;SRVR:BN8PR15MB3202;H:BN8PR15MB2626.namprd15.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;MX:1;A:1;
received-spf: None (protection.outlook.com: fb.com does not designate
 permitted sender hosts)
x-ms-exchange-senderadcheck: 1
x-microsoft-antispam-message-info: fbbiu9IS8c9cjLlrrPWGb4/swivMENAbfzLWkFjkB/LRTWPtLTm52RUCQP6aNPqLNFcgyDpURK7wS4epIjNRVg1DxBIlRj0ptDQalOSSszAdVVGalt9Y0hWwNN1M3co+g5pz4ovIuIttq6f5/UoC7K7H3P08gzbXnTfW6cLAfo3afZTA8Ezq4tnWNtm9mx3sX9uFRstXrskd6IsU/kqAP9elwsnRdFwxpj7b/TtM1mtc99jbFz1Xz2jDDRZZccaHDZQnlkXNW7lx/nbxUkLrssV3ijjJ3rDBj7cgpVHuD5vuIJaG5xCtgQEOR9dRfGMRufedq83aGVzoSWgXYVVxEwg3hlabim8Omwg0r7EuEQgtolAPMEkLCFLu2igJdfMI/ZruHqz5a2DH1dS9SKw295JMvmkm+9H7YKRikkOPDXc=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <DFE5A7730F46444C9CFC8429E91978C9@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 83837ae1-4780-4444-d99e-08d6f858c153
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Jun 2019 04:02:20.6980
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: guro@fb.com
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR15MB3202
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-06-24_04:,,
 signatures=0
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 priorityscore=1501
 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0
 clxscore=1015 lowpriorityscore=0 mlxscore=0 impostorscore=0
 mlxlogscore=898 adultscore=0 classifier=spam adjust=0 reason=mlx
 scancount=1 engine=8.0.1-1810050000 definitions=main-1906240032
X-FB-Internal: deliver
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sun, Jun 23, 2019 at 08:29:21PM -0700, Alexei Starovoitov wrote:
> On 6/23/19 7:30 PM, Roman Gushchin wrote:
> > Since commit 4bfc0bb2c60e ("bpf: decouple the lifetime of cgroup_bpf
> > from cgroup itself"), cgroup_bpf release occurs asynchronously
> > (from a worker context), and before the release of the cgroup itself.
> >=20
> > This introduced a previously non-existing race between the release
> > and update paths. E.g. if a leaf's cgroup_bpf is released and a new
> > bpf program is attached to the one of ancestor cgroups at the same
> > time. The race may result in double-free and other memory corruptions.
> >=20
> > To fix the problem, let's protect the body of cgroup_bpf_release()
> > with cgroup_mutex, as it was effectively previously, when all this
> > code was called from the cgroup release path with cgroup mutex held.
> >=20
> > Also make sure, that we don't leave already freed pointers to the
> > effective prog arrays. Otherwise, they can be released again by
> > the update path. It wasn't necessary before, because previously
> > the update path couldn't see such a cgroup, as cgroup_bpf and cgroup
> > itself were released together.
>=20
> I thought dying cgroup won't have any children cgroups ?

It's not completely true, a dying cgroup can't have living children.

> It should have been empty with no tasks inside it?

Right.

> Only some resources are still held?

Right.

> mutex and zero init are highly suspicious.
> It feels that cgroup_bpf_release is called too early.

An alternative solution is to bump the refcounter on
every update path, and explicitly skip de-bpf'ed cgroups.

>=20
> Thinking from another angle... if child cgroups can still attach then
> this bpf_release is broken.

Hm, what do you mean under attach? It's not possible to attach
a new prog, but if a prog is attached to a parent cgroup,
a pointer can spill through "effective" array.

But I agree, it's broken. Update path should ignore such
cgroups (cgroups, which cgroup_bpf was released). I'll take a look.

> The code should be
> calling __cgroup_bpf_detach() one by one to make sure
> update_effective_progs() is called, since descendant are still
> sort-of alive and can attach?

Not sure I get you. Dying cgroup is a leaf cgroup.

>=20
> My money is on 'too early'.
> May be cgroup is not dying ?
> Just cgroup_sk_free() is called on the last socket and
> this auto-detach logic got triggered incorrectly?

So, once again, what's my picture:

A
A/B
A/B/C

cpu1:                               cpu2:
rmdir C                             attach new prog to A
C got dying                         update A, update B, update C...
C's cgroup_bpf is released          C's effective progs is replaced with ne=
w one
                                    old is double freed

It looks like it can be reproduced without any sockets.

Thanks!