Linux内核上游提交完整流程及示例的后续上游反馈-编程知识

接前篇文章：Linux内核上游提交完整流程及示例

在Linux内核上游提交完整流程及示例中，笔者做了一次针对于DRM的上游提交。起初以为需要几天、一周甚至更长的时间才会有回复，没想到在24小时内就得到了上游的回复，内容如下：

可以看到，上游维护者Ville Syrjälä给出的反馈是：

Not in this case since we already checked it earlier.

但实际上结合代码来看，Linux Kernel源码根目录的drivers/gpu/drm/drm_framebuffer.c中的framebuffer_check函数的代码原本如下：

static int framebuffer_check(struct drm_device *dev,const struct drm_mode_fb_cmd2 *r)
{const struct drm_format_info *info;int i;/* check if the format is supported at all */if (!__drm_format_info(r->pixel_format)) {drm_dbg_kms(dev, "bad framebuffer format %p4cc\n",&r->pixel_format);return -EINVAL;}if (r->width == 0) {drm_dbg_kms(dev, "bad framebuffer width %u\n", r->width);return -EINVAL;}if (r->height == 0) {drm_dbg_kms(dev, "bad framebuffer height %u\n", r->height);return -EINVAL;}/* now let the driver pick its own format info */info = drm_get_format_info(dev, r);for (i = 0; i < info->num_planes; i++) {unsigned int width = fb_plane_width(r->width, info, i);unsigned int height = fb_plane_height(r->height, info, i);unsigned int block_size = info->char_per_block[i];u64 min_pitch = drm_format_info_min_pitch(info, i, width);if (!block_size && (r->modifier[i] == DRM_FORMAT_MOD_LINEAR)) {drm_dbg_kms(dev, "Format requires non-linear modifier for plane %d\n", i);return -EINVAL;}if (!r->handles[i]) {drm_dbg_kms(dev, "no buffer object handle for plane %d\n", i);return -EINVAL;}if (min_pitch > UINT_MAX)return -ERANGE;if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)return -ERANGE;if (block_size && r->pitches[i] < min_pitch) {drm_dbg_kms(dev, "bad pitch %u for plane %d\n", r->pitches[i], i);return -EINVAL;}if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) {drm_dbg_kms(dev, "bad fb modifier %llu for plane %d\n",r->modifier[i], i);return -EINVAL;}if (r->flags & DRM_MODE_FB_MODIFIERS &&r->modifier[i] != r->modifier[0]) {drm_dbg_kms(dev, "bad fb modifier %llu for plane %d\n",r->modifier[i], i);return -EINVAL;}/* modifier specific checks: */switch (r->modifier[i]) {case DRM_FORMAT_MOD_SAMSUNG_64_32_TILE:/* NOTE: the pitch restriction may be lifted later if it turns* out that no hw has this restriction:*/if (r->pixel_format != DRM_FORMAT_NV12 ||width % 128 || height % 32 ||r->pitches[i] % 128) {drm_dbg_kms(dev, "bad modifier data for plane %d\n", i);return -EINVAL;}break;default:break;}}for (i = info->num_planes; i < 4; i++) {if (r->modifier[i]) {drm_dbg_kms(dev, "non-zero modifier for unused plane %d\n", i);return -EINVAL;}/* Pre-FB_MODIFIERS userspace didn't clear the structs properly. */if (!(r->flags & DRM_MODE_FB_MODIFIERS))continue;if (r->handles[i]) {drm_dbg_kms(dev, "buffer object handle for unused plane %d\n", i);return -EINVAL;}if (r->pitches[i]) {drm_dbg_kms(dev, "non-zero pitch for unused plane %d\n", i);return -EINVAL;}if (r->offsets[i]) {drm_dbg_kms(dev, "non-zero offset for unused plane %d\n", i);return -EINVAL;}}return 0;
}

而其中的代码片段：

    /* now let the driver pick its own format info */info = drm_get_format_info(dev, r);for (i = 0; i < info->num_planes; i++) {……

info确实没有做参数检查。并且从以上代码的上文来看，在framebuffer_check函数开始定义了info之后（代码如下），

const struct drm_format_info *info;

再次使用到info的地方就是上边的那一代码片段，期间并没有任何使用的地方，也就是说上游维护者Ville Syrjälä给出的反馈

Not in this case since we already checked it earlier.

在上文中不成立。

再来看代码的下文。drm_get_format_info函数（在drivers/gpu/drm/drm_fourcc.c中）代码如下：

/*** drm_get_format_info - query information for a given framebuffer configuration* @dev: DRM device* @mode_cmd: metadata from the userspace fb creation request** Returns:* The instance of struct drm_format_info that describes the pixel format, or* NULL if the format is unsupported.*/
const struct drm_format_info *
drm_get_format_info(struct drm_device *dev,const struct drm_mode_fb_cmd2 *mode_cmd)
{const struct drm_format_info *info = NULL;if (dev->mode_config.funcs->get_format_info)info = dev->mode_config.funcs->get_format_info(mode_cmd);if (!info)info = drm_format_info(mode_cmd->pixel_format);return info;
}
EXPORT_SYMBOL(drm_get_format_info);

笔者经过进一步跟进，无论是

info = dev->mode_config.funcs->get_format_info(mode_cmd);

还是

info = drm_format_info(mode_cmd->pixel_format);

所调用的函数中，也没有检查info、保证其不为空的代码。因此，上游维护者的回复，笔者并不认同。于是笔者也没客气，直接针对于他的回复邮件又回复了邮件：

这次的回复就没有下文了。

后来，笔者又在DRM代码中发现了一处问题。还是在drivers/gpu/drm/drm_framebuffer.c的framebuffer_check函数中，以下代码片段：

    for (i = info->num_planes; i < 4; i++) {if (r->modifier[i]) {drm_dbg_kms(dev, "non-zero modifier for unused plane %d\n", i);return -EINVAL;}/* Pre-FB_MODIFIERS userspace didn't clear the structs properly. */if (!(r->flags & DRM_MODE_FB_MODIFIERS))continue;if (r->handles[i]) {drm_dbg_kms(dev, "buffer object handle for unused plane %d\n", i);return -EINVAL;}if (r->pitches[i]) {drm_dbg_kms(dev, "non-zero pitch for unused plane %d\n", i);return -EINVAL;}if (r->offsets[i]) {drm_dbg_kms(dev, "non-zero offset for unused plane %d\n", i);return -EINVAL;}}

其中以下一行

 for (i = info->num_planes; i < 4; i++) {

直接使用了数字4，导致可移植性有问题。于是笔者改为了

for (i = info->num_planes; i < DRM_FORMAT_MAX_PLANES; i++) {

DRM_FORMAT_MAX_PLANES宏在include/drm/drm_fourcc.h中定义：

/*** DRM_FORMAT_MAX_PLANES - maximum number of planes a DRM format can have*/
#define DRM_FORMAT_MAX_PLANES	4u

再次按照Linux内核上游提交完整流程及示例中的步骤进行上游提交尝试。这次也是在24小时之内得到了回复。第一个回复如下：

“reviewed”了！离DRM即内核上游提交就差一步了！正当笔者高兴之际，第二天迎来了当头一棒。又一位维护者回复了此patch：

Hi Am 02.11.23 um 03:29 schrieb Peng Hao: > Use Macro DRM_FORMAT_MAX_PLANES instead of 4, to improve modifiability. > > Signed-off-by: Peng Hao <penghao@dingdao.com> > --- > drivers/gpu/drm/drm_framebuffer.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > index 2dd97473ca10..bf283dae9090 100644 > --- a/drivers/gpu/drm/drm_framebuffer.c > +++ b/drivers/gpu/drm/drm_framebuffer.c > @@ -254,7 +254,7 @@ static int framebuffer_check(struct drm_device *dev, > } > } > > - for (i = info->num_planes; i < 4; i++) { > + for (i = info->num_planes; i < DRM_FORMAT_MAX_PLANES; i++) { This change makes the code more fragile. '4' is a fixed constant in the UAPI struct, while DRM_FORMAT_MAX_PLANES is an internal constant. I agree that both should reasonably have the same value. But (potentially) changing the value of DRM_FORMAT_MAX_PLANES will break these loops with a possible OOB access. To make make this code more robust, it might be better to rewrite the tests like this for (i = num_planes; i < ARRAY_SIZE(r->modifier); +i) { // the test for modifier[i] } if (r->flags & DRM_MODE_FB_MODIFIERS) { for (i < ARRAY_SIZE(handles)) { // test for handles[i] } for (i < ARRAY_SIZE(pitches)) { // test for pitches[i] } for (i < ARRAY_SIZE(offsets)) { // test for offsets[i] } } Best regards Thomas > if (r->modifier[i]) { > drm_dbg_kms(dev, "non-zero modifier for unused plane %d\n", i); > return -EINVAL; -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg)

这位维护者回复得比较有道理

看来真正想实现上游提交，还得自身知识和技术过硬才行。不能只看到一小处就以为“捡了漏”。