Re: [PATCH v13 24/27] x86/resctrl: Introduce the interface to modify assignments in a group

["Moger, Babu" <babu.moger@amd.com>]

Hi Peter,


On 5/26/25 04:48, Peter Newman wrote:
> Hi Babu,
> 
> On Fri, May 16, 2025 at 12:56 AM Babu Moger <babu.moger@amd.com> wrote:
> 
>> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
>> index 8d970b99bbbd..ea1782723f81 100644
>> --- a/fs/resctrl/rdtgroup.c
>> +++ b/fs/resctrl/rdtgroup.c
>> @@ -2126,6 +2126,168 @@ static int mbm_L3_assignments_show(struct kernfs_open_file *of, struct seq_file
>>         return ret;
>>  }
>>
>> +/*
>> + * mbm_get_mon_event_by_name() - Return the mon_evt entry for the matching
>> + * event name.
>> + */
>> +static struct mon_evt *mbm_get_mon_event_by_name(struct rdt_resource *r,
>> +                                                char *name)
>> +{
>> +       struct mon_evt *mevt;
>> +
>> +       list_for_each_entry(mevt, &r->mon.evt_list, list) {
>> +               if (!strcmp(mevt->name, name))
>> +                       return mevt;
>> +       }
>> +
>> +       return NULL;
>> +}
>> +
>> +static unsigned int resctrl_get_assing_type(char *assign)
>> +{
>> +       unsigned int mon_state = ASSIGN_NONE;
>> +       int len = strlen(assign);
> 
> [  395.013183] BUG: kernel NULL pointer dereference, address: 0000000000000000
> [  395.013426] #PF: supervisor read access in kernel mode
> [  395.013600] #PF: error_code(0x0000) - not-present page
> [  395.013779] PGD 39322c067 P4D 2a4f49067 PUD 2a4f4a067 PMD 0
> [  395.013973] Oops: Oops: 0000 [#1] SMP DEBUG_PAGEALLOC NOPTI
> [  395.014156] CPU: 37 UID: 0 PID: 24147 Comm: bash Not tainted
> 6.15.0-dbg-DEV #13 NONE
> [  395.014403] Hardware name: Google Astoria-Turin/astoria, BIOS
> 0.20241223.2-0 01/17/2025
> [  395.014652] RIP: 0010:strlen+0xb/0x20
> [  395.014778] Code: 66 66 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 90
> 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa 48 c7 c0 ff
> ff ff ff <80> 7c 07 01 00 48 8d 40 01 75 f5 c3 cc cc cc cc cc 0f 1f 40
> 00 90
> [  395.015356] RSP: 0018:ffa000002f743d58 EFLAGS: 00010246
> [  395.015522] RAX: ffffffffffffffff RBX: ff11000129a00600 RCX: 0000000000000000
> [  395.015747] RDX: ff110001299f5253 RSI: ffffffff827b9651 RDI: 0000000000000000
> [  395.015968] RBP: 0000000000000000 R08: 000000000000003d R09: 0000000000000000
> [  395.016202] R10: ffffffff827b9652 R11: 0000000000000000 R12: ffffffff8305b7f8
> [  395.016421] R13: ff110001299f5240 R14: 0000000000000014 R15: 0000000000000000
> [  395.016644] FS:  00007f1281ff8b80(0000) GS:ff1100bdc8276000(0000)
> knlGS:0000000000000000
> [  395.016893] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  395.017071] CR2: 0000000000000000 CR3: 0000000420bc8002 CR4: 0000000000771ef0
> [  395.017298] PKRU: 55555554
> [  395.017388] Call Trace:
> [  395.017471]  <TASK>
> [  395.017545]  mbm_L3_assignments_write+0x2d4/0x4e0
> [  395.017700]  kernfs_fop_write_iter+0x132/0x1c0
> [  395.017851]  vfs_write+0x2bf/0x3c0
> [  395.017963]  ksys_write+0x82/0x100
> [  395.018074]  do_syscall_64+0xee/0x210
> [  395.018198]  ? exc_page_fault+0x81/0xe0
> [  395.018321]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> [  395.018482] RIP: 0033:0x7f128177f8b3
> [  395.018598] Code: cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc
> cc cc cc cc cc cc cc 48 8b 05 99 91 07 00 83 38 00 75 10 b8 01 00 00
> 00 0f 05 <48> 3d 01 f0 ff ff 73 4d c3 55 48 89 e5 41 57 41 56 53 50 48
> 89 d3
> [  395.019167] RSP: 002b:00007ffff66e80f8 EFLAGS: 00000246 ORIG_RAX:
> 0000000000000001
> [  395.019409] RAX: ffffffffffffffda RBX: 0000000000000014 RCX: 00007f128177f8b3
> [  395.019636] RDX: 0000000000000014 RSI: 0000000001eedb60 RDI: 0000000000000001
> [  395.019861] RBP: 00007ffff66e8120 R08: 0000000000000000 R09: 0000000000000000
> [  395.020081] R10: 00007ffff66e81b0 R11: 0000000000000246 R12: 0000000001eedb60
> [  395.020303] R13: 0000000000000001 R14: 00007f12817fa650 R15: 0000000000000014
> [  395.020532]  </TASK>
> 

Yes. Got it. Missing NULL check. Simplified the function now. Thanks


static unsigned int resctrl_get_assign_type(char *assign)
{

        if (!assign || strlen(assign) != 1)
                return ASSIGN_INVALID;

        switch (*assign) {
        case 'e':
                return ASSIGN_EXCLUSIVE;
        case '_':
                return ASSIGN_NONE;
        default:
                return ASSIGN_INVALID;
        }
}


>> +
>> +       if (!len || len > 1)
>> +               return ASSIGN_INVALID;
>> +
>> +       switch (*assign) {
>> +       case 'e':
>> +               mon_state = ASSIGN_EXCLUSIVE;
>> +               break;
>> +       case '_':
>> +               mon_state = ASSIGN_NONE;
>> +               break;
>> +       default:
>> +               mon_state = ASSIGN_INVALID;
>> +               break;
>> +       }
>> +
>> +       return mon_state;
>> +}
>> +
>> +static int resctrl_process_assign(struct rdt_resource *r, struct rdtgroup *rdtgrp,
>> +                                 char *config, char *tok)
>> +{
>> +       struct rdt_mon_domain *d;
>> +       char *dom_str, *id_str;
>> +       unsigned long dom_id = 0;
>> +       struct mon_evt *mevt;
>> +       int assign_type;
>> +       char domain[10];
>> +       bool found;
>> +       int ret;
>> +
>> +       mevt = mbm_get_mon_event_by_name(r, config);
>> +       if (!mevt) {
>> +               rdt_last_cmd_printf("Invalid assign configuration %s\n", config);
>> +               return  -ENOENT;
>> +       }
>> +
>> +next:
>> +       if (!tok || tok[0] == '\0')
>> +               return 0;
>> +
>> +       /* Start processing the strings for each domain */
>> +       dom_str = strim(strsep(&tok, ";"));
>> +
>> +       id_str = strsep(&dom_str, "=");
> 
> If there's no '=' then dom_str becomes NULL...

Yea. That is correct.

> 
>> +
>> +       /* Check for domain id '*' which means all domains */
>> +       if (id_str && *id_str == '*') {
>> +               d = NULL;
>> +               goto check_state;
>> +       } else if (!id_str || kstrtoul(id_str, 10, &dom_id)) {
>> +               rdt_last_cmd_puts("Missing domain id\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       /* Verify if the dom_id is valid */
>> +       found = false;
>> +       list_for_each_entry(d, &r->mon_domains, hdr.list) {
>> +               if (d->hdr.id == dom_id) {
>> +                       found = true;
>> +                       break;
>> +               }
>> +       }
>> +
>> +       if (!found) {
>> +               rdt_last_cmd_printf("Invalid domain id %ld\n", dom_id);
>> +               return -EINVAL;
>> +       }
>> +
>> +check_state:
>> +       assign_type = resctrl_get_assing_type(dom_str);
> 
> then the resulting type of whatever this is supposed to mean is "panic"
> 
> Thanks,
> -Peter
> 

-- 
Thanks
Babu Moger